RepeatedField: unset by index (protocolbuffers#11425)

Instead of using `array_pop()` to remove last element use `unset()` to remove by index Closes protocolbuffers#11425 COPYBARA_INTEGRATE_REVIEW=protocolbuffers#11425 from Leprechaunz:unset-by-index a47fba5 PiperOrigin-RevId: 508691545
alielbashir · Feb 14, 2023 · c51d50a · c51d50a
1 parent 3dcefa5
commit c51d50a
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 4 deletions.
diff --git a/php/ext/google/protobuf/array.c b/php/ext/google/protobuf/array.c
@@ -406,13 +406,13 @@ PHP_METHOD(RepeatedField, offsetUnset) {
     return;
   }
 
-  if (size == 0 || index != size - 1) {
+  if (size == 0 || index < 0 || index >= size) {
     php_error_docref(NULL, E_USER_ERROR, "Cannot remove element at %ld.\n",
                      index);
     return;
   }
 
-  upb_Array_Resize(intern->array, size - 1, Arena_Get(&intern->arena));
+  upb_Array_Delete(intern->array, index, 1);
 }
 
 /**

diff --git a/php/src/Google/Protobuf/Internal/RepeatedField.php b/php/src/Google/Protobuf/Internal/RepeatedField.php
@@ -219,13 +219,13 @@ public function offsetSet($offset, $value)
     public function offsetUnset($offset)
     {
         $count = count($this->container);
-        if (!is_numeric($offset) || $count === 0 || $offset !== $count - 1) {
+        if (!is_numeric($offset) || $count === 0 || $offset < 0 || $offset >= $count) {
             trigger_error(
                 "Cannot remove element at the given index",
                 E_USER_ERROR);
             return;
         }
-        array_pop($this->container);
+        array_splice($this->container, $offset, 1);
     }
 
     /**

diff --git a/php/tests/ArrayTest.php b/php/tests/ArrayTest.php
@@ -655,4 +655,31 @@ public function testClone()
         $arr2 = clone $arr;
         $this->assertSame($arr[0], $arr2[0]);
     }
+
+    #########################################################
+    # Test offsetUnset
+    #########################################################
+
+    public function testOffsetUnset()
+    {
+        $arr = new RepeatedField(GPBType::INT32);
+        $arr[] = 0;
+        $arr[] = 1;
+        $arr[] = 2;
+
+        $this->assertSame(3, count($arr));
+        $this->assertSame(0, $arr[0]);
+        $this->assertSame(1, $arr[1]);
+        $this->assertSame(2, $arr[2]);
+
+        $arr->offsetUnset(1);
+        $this->assertSame(0, $arr[0]);
+        $this->assertSame(2, $arr[1]);
+
+        $arr->offsetUnset(0);
+        $this->assertSame(2, $arr[0]);
+
+        $arr->offsetUnset(0);
+        $this->assertCount(0, $arr);
+    }
 }