#310 - Array builtin completeness #561
Conversation
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-561 |
There was a problem hiding this comment.
Thank you!
Maybe we can rename the two binary_search* variants to bsearch* (to stay with Godot terms), because Rust's own slice::binary_search() has a different signature.
For comments, please use the available width of 120-145 chars, even if some existing code doesn't follow that.
Great tests! 👍
godot-core/src/builtin/array.rs
Outdated
| /// Note: The sorting algorithm used is not | ||
| /// [stable](https://en.wikipedia.org/wiki/Sorting_algorithm#Stability). This means that values | ||
| /// considered equal may have their order changed when using `sort_unstable`. |
There was a problem hiding this comment.
You can use the 120-145 available width:
| /// Note: The sorting algorithm used is not | |
| /// [stable](https://en.wikipedia.org/wiki/Sorting_algorithm#Stability). This means that values | |
| /// considered equal may have their order changed when using `sort_unstable`. | |
| /// Note: the sorting algorithm used is not [stable](https://en.wikipedia.org/wiki/Sorting_algorithm#Stability). | |
| /// This means that values considered equal may have their order changed when using `sort_unstable`. |
| let res = i32::from_variant(args[0]) > i32::from_variant(args[1]); | ||
| Ok(Variant::from(res)) | ||
| }); | ||
| assert_eq!(a.clone().binary_search_custom(&1, func.clone()), 3); |
There was a problem hiding this comment.
Why the clone here? Maybe express the intention with a short comment before.
There was a problem hiding this comment.
a.clone() was a carryover from the array implementation. Didn't catch that it wasn't required anymore because .binary_search_custom() is immutable.
func.clone() is probably easier to understand. I can add a comment there if you think its worthwhile to clarify, but the compiler will get mad if its gone. Use after move and all that.
|
Also, code that relies on You can apply this to both |
What about the case of |
Keep it because "sort" in Rust also has a meaning: stable sort. |
| /// Finds the index of an existing value in a sorted array using binary search. | ||
| /// Equivalent of `bsearch` in GDScript. | ||
| /// | ||
| /// If the value is not present in the array, returns the insertion index that would maintain | ||
| /// sorting order. | ||
| /// If the value is not present in the array, returns the insertion index that | ||
| /// would maintain sorting order. | ||
| /// | ||
| /// Calling `binary_search` on an unsorted array results in unspecified behavior. | ||
| pub fn binary_search(&self, value: &T) -> usize { | ||
| /// Calling `bsearch` on an unsorted array results in unspecified behavior. |
There was a problem hiding this comment.
Generally, we try to keep 120-145 characters, see here.
It's not important enough to block this PR, but maybe think of it next time 🙂
This will make
Arrayin #310 complete.There would be additional work for
ArrayandDictionaryif we are interested in addingread_onlysemantics.