Consistent by-ref and by-value parameters for Packed*Array

Author: Bromeon

godot-core/src/builtin/array.rs

@@ -242,6 +242,7 @@ impl<T: ArrayElement> Array<T> {
         // SAFETY: The array has type `T` and we're writing a value of type `T` to it.
         unsafe { self.as_inner_mut() }.push_front(value.to_variant());
     }
+
     /// Removes and returns the last element of the array. Returns `None` if the array is empty.
     ///
     /// _Godot equivalent: `pop_back`_
@@ -266,7 +267,7 @@ impl<T: ArrayElement> Array<T> {
         })
     }
 
-    /// Inserts a new element before the index. The index must be valid or the end of the array (`index == len()`).
+    /// ⚠️ Inserts a new element before the index. The index must be valid or the end of the array (`index == len()`).
     ///
     /// On large arrays, this method is much slower than [`push()`][Self::push], as it will move all the array's elements after the inserted element.
     /// The larger the array, the slower `insert()` will be.
@@ -286,12 +287,13 @@ impl<T: ArrayElement> Array<T> {
 
     /// ⚠️ Removes and returns the element at the specified index. Equivalent of `pop_at` in GDScript.
     ///
-    /// On large arrays, this method is much slower than `pop_back()` as it will move all the array's
+    /// On large arrays, this method is much slower than [`pop()`][Self::pop] as it will move all the array's
     /// elements after the removed element. The larger the array, the slower `remove()` will be.
     ///
     /// # Panics
     ///
     /// If `index` is out of bounds.
+    #[doc(alias = "pop_at")]
     pub fn remove(&mut self, index: usize) -> T {
         self.check_bounds(index);
 
@@ -304,7 +306,7 @@ impl<T: ArrayElement> Array<T> {
     ///
     /// If the value does not exist in the array, nothing happens. To remove an element by index, use [`remove()`][Self::remove] instead.
     ///
-    /// On large arrays, this method is much slower than [`pop_back()`][Self::pop_back], as it will move all the array's
+    /// On large arrays, this method is much slower than [`pop()`][Self::pop], as it will move all the array's
     /// elements after the removed element.
     pub fn erase(&mut self, value: &T) {
         // SAFETY: We don't write anything to the array.

godot-core/src/builtin/packed_array.rs

@@ -101,13 +101,13 @@ macro_rules! impl_packed_array {
             ///
             /// _Godot equivalent: `has`_
             #[doc(alias = "has")]
-            pub fn contains(&self, value: $Element) -> bool {
-                self.as_inner().has(Self::into_arg(value))
+            pub fn contains(&self, value: &$Element) -> bool {
+                self.as_inner().has(Self::to_arg(value))
             }
 
             /// Returns the number of times a value is in the array.
-            pub fn count(&self, value: $Element) -> usize {
-                to_usize(self.as_inner().count(Self::into_arg(value)))
+            pub fn count(&self, value: &$Element) -> usize {
+                to_usize(self.as_inner().count(Self::to_arg(value)))
             }
 
             /// Returns the number of elements in the array. Equivalent of `size()` in Godot.
@@ -276,9 +276,9 @@ macro_rules! impl_packed_array {
             /// Searches the array for the first occurrence of a value and returns its index, or
             /// `None` if not found. Starts searching at index `from`; pass `None` to search the
             /// entire array.
-            pub fn find(&self, value: $Element, from: Option<usize>) -> Option<usize> {
+            pub fn find(&self, value: &$Element, from: Option<usize>) -> Option<usize> {
                 let from = to_i64(from.unwrap_or(0));
-                let index = self.as_inner().find(Self::into_arg(value), from);
+                let index = self.as_inner().find(Self::to_arg(value), from);
                 if index >= 0 {
                     Some(index.try_into().unwrap())
                 } else {
@@ -289,9 +289,9 @@ macro_rules! impl_packed_array {
             /// Searches the array backwards for the last occurrence of a value and returns its
             /// index, or `None` if not found. Starts searching at index `from`; pass `None` to
             /// search the entire array.
-            pub fn rfind(&self, value: $Element, from: Option<usize>) -> Option<usize> {
+            pub fn rfind(&self, value: &$Element, from: Option<usize>) -> Option<usize> {
                 let from = from.map(to_i64).unwrap_or(-1);
-                let index = self.as_inner().rfind(Self::into_arg(value), from);
+                let index = self.as_inner().rfind(Self::to_arg(value), from);
                 // It's not documented, but `rfind` returns -1 if not found.
                 if index >= 0 {
                     Some(to_usize(index))
@@ -305,13 +305,13 @@ macro_rules! impl_packed_array {
             /// If the value is not present in the array, returns the insertion index that would maintain sorting order.
             ///
             /// Calling `bsearch()` on an unsorted array results in unspecified (but safe) behavior.
-            pub fn bsearch(&self, value: $Element) -> usize {
-                to_usize(self.as_inner().bsearch(Self::into_arg(value), true))
+            pub fn bsearch(&self, value: &$Element) -> usize {
+                to_usize(self.as_inner().bsearch(Self::to_arg(value), true))
             }
 
             #[deprecated = "Renamed to bsearch like in Godot, to avoid confusion with Rust's slice::binary_search."]
             pub fn binary_search(&self, value: $Element) -> usize {
-                self.bsearch(value)
+                self.bsearch(&value)
             }
 
             /// Reverses the order of the elements in the array.
@@ -386,6 +386,12 @@ macro_rules! impl_packed_array {
                 e.into()
             }
 
+            #[inline]
+            fn to_arg(e: &$Element) -> $Arg {
+                // Once PackedArra<T> is generic, this could use a better tailored implementation that may not need to clone.
+                e.clone().into()
+            }
+
             #[doc(hidden)]
             pub fn as_inner(&self) -> inner::$Inner<'_> {
                 inner::$Inner::from_outer(self)

itest/rust/src/builtin_tests/containers/packed_array_test.rs

@@ -67,7 +67,7 @@ fn packed_array_clone() {
     let mut array = PackedByteArray::from(&[1, 2]);
     #[allow(clippy::redundant_clone)]
     let clone = array.clone();
-    array.set(0, 3);
+    array[0] = 3;
 
     assert_eq!(clone[0], 1);
 }
@@ -159,41 +159,29 @@ fn packed_array_get() {
 fn packed_array_binary_search() {
     let array = PackedByteArray::from(&[1, 3]);
 
-    assert_eq!(array.binary_search(0), 0);
-    assert_eq!(array.binary_search(1), 0);
-    assert_eq!(array.binary_search(2), 1);
-    assert_eq!(array.binary_search(3), 1);
-    assert_eq!(array.binary_search(4), 2);
+    assert_eq!(array.bsearch(&0), 0);
+    assert_eq!(array.bsearch(&1), 0);
+    assert_eq!(array.bsearch(&2), 1);
+    assert_eq!(array.bsearch(&3), 1);
+    assert_eq!(array.bsearch(&4), 2);
 }
 
 #[itest]
 fn packed_array_find() {
     let array = PackedByteArray::from(&[1, 2, 1]);
 
-    assert_eq!(array.find(0, None), None);
-    assert_eq!(array.find(1, None), Some(0));
-    assert_eq!(array.find(1, Some(1)), Some(2));
+    assert_eq!(array.find(&0, None), None);
+    assert_eq!(array.find(&1, None), Some(0));
+    assert_eq!(array.find(&1, Some(1)), Some(2));
 }
 
 #[itest]
 fn packed_array_rfind() {
     let array = PackedByteArray::from(&[1, 2, 1]);
 
-    assert_eq!(array.rfind(0, None), None);
-    assert_eq!(array.rfind(1, None), Some(2));
-    assert_eq!(array.rfind(1, Some(1)), Some(0));
-}
-
-#[itest]
-fn packed_array_set() {
-    let mut array = PackedByteArray::from(&[1, 2]);
-
-    array.set(0, 3);
-    assert_eq!(array[0], 3);
-
-    expect_panic("Array index 2 out of bounds: length is 2", move || {
-        array.set(2, 4);
-    });
+    assert_eq!(array.rfind(&0, None), None);
+    assert_eq!(array.rfind(&1, None), Some(2));
+    assert_eq!(array.rfind(&1, Some(1)), Some(0));
 }
 
 #[itest]

itest/rust/src/register_tests/constant_test.rs

@@ -89,7 +89,7 @@ fn constants_correct_value() {
         .done();
 
     for (constant_name, constant_value) in CONSTANTS {
-        assert!(constants.contains(constant_name.into()));
+        assert!(constants.contains(&constant_name.into()));
         assert_eq!(
             ClassDb::singleton().class_get_integer_constant(
                 HasConstants::class_name().to_string_name(),
@@ -216,11 +216,11 @@ macro_rules! test_enum_export {
                 .done();
 
             for (variant_name, variant_value) in variants {
-                assert!(godot_variants.contains(variant_name.into()));
-                assert!(constants.contains(variant_name.into()));
+                let variant_name = GString::from(variant_name);
+                assert!(godot_variants.contains(&variant_name));
+                assert!(constants.contains(&variant_name));
                 assert_eq!(
-                    ClassDb::singleton()
-                        .class_get_integer_constant(class_name.to_string_name(), variant_name.into()),
+                    ClassDb::singleton().class_get_integer_constant(class_name.to_string_name(), variant_name.into()),
                     variant_value
                 );
             }