Rename bottom/top to first/last, refactor

TysonAndre · TysonAndre · commit f57f804660bc · 2022-10-10T16:37:41.000-04:00
diff --git a/ext/collections/collections_deque.c b/ext/collections/collections_deque.c
@@ -139,17 +139,6 @@ static collections_deque *collections_deque_from_object(zend_object *obj)
 
 #define Z_DEQUE_P(zv)  collections_deque_from_object(Z_OBJ_P((zv)))
 
-/* Helps enforce the invariants in debug mode:
- *   - if size == 0, then circular_buffer == NULL
- *   - if size > 0, then circular_buffer != NULL
- *   - size is not less than 0
- */
-static bool collections_deque_entries_empty_size(const collections_deque_entries *array)
-{
-	DEBUG_ASSERT_CONSISTENT_DEQUE(array);
-	return array->size == 0;
-}
-
 static bool collections_deque_entries_empty_capacity(const collections_deque_entries *array)
 {
 	DEBUG_ASSERT_CONSISTENT_DEQUE(array);
@@ -760,6 +749,7 @@ static zend_array* collections_deque_to_new_array(const collections_deque_entrie
 		do {
 			Z_TRY_ADDREF_P(p);
 			ZEND_HASH_FILL_ADD(p);
+			p++;
 			if (p == end) {
 				p = circular_buffer;
 			}
@@ -769,20 +759,6 @@ static zend_array* collections_deque_to_new_array(const collections_deque_entrie
 	return values;
 }
 
-PHP_METHOD(Collections_Deque, __serialize)
-{
-	ZEND_PARSE_PARAMETERS_NONE();
-
-	collections_deque *intern = Z_DEQUE_P(ZEND_THIS);
-
-	if (collections_deque_entries_empty_size(&intern->array)) {
-		RETURN_EMPTY_ARRAY();
-	}
-	/* Unlike FixedArray, there's no setSize, so there's no reason to delete indexes */
-
-	RETURN_ARR(collections_deque_to_new_array(&intern->array));
-}
-
 PHP_METHOD(Collections_Deque, toArray)
 {
 	ZEND_PARSE_PARAMETERS_NONE();
@@ -1087,14 +1063,14 @@ PHP_METHOD(Collections_Deque, pop)
 	collections_deque_try_shrink_capacity(intern, old_size);
 }
 
-PHP_METHOD(Collections_Deque, top)
+PHP_METHOD(Collections_Deque, last)
 {
 	ZEND_PARSE_PARAMETERS_NONE();
 
 	const collections_deque *intern = Z_DEQUE_P(ZEND_THIS);
 	const size_t old_size = intern->array.size;
 	if (old_size == 0) {
-		zend_throw_exception(spl_ce_UnderflowException, "Cannot read top of empty Collections\\Deque", 0);
+		zend_throw_exception(spl_ce_UnderflowException, "Cannot read last value of empty Collections\\Deque", 0);
 		RETURN_THROWS();
 	}
 
@@ -1126,14 +1102,14 @@ PHP_METHOD(Collections_Deque, shift)
 	collections_deque_try_shrink_capacity(intern, old_size);
 }
 
-PHP_METHOD(Collections_Deque, bottom)
+PHP_METHOD(Collections_Deque, first)
 {
 	ZEND_PARSE_PARAMETERS_NONE();
 
 	const collections_deque *intern = Z_DEQUE_P(ZEND_THIS);
 	DEBUG_ASSERT_CONSISTENT_DEQUE(&intern->array);
 	if (intern->array.size == 0) {
-		zend_throw_exception(spl_ce_UnderflowException, "Cannot read bottom of empty Collections\\Deque", 0);
+		zend_throw_exception(spl_ce_UnderflowException, "Cannot read first value of empty Collections\\Deque", 0);
 		RETURN_THROWS();
 	}
 
@@ -1152,45 +1128,6 @@ PHP_METHOD(Collections_Deque, offsetUnset)
 	RETURN_THROWS();
 }
 
-static void collections_deque_return_list(zval *return_value, const collections_deque *intern)
-{
-	// Can't use collections_convert_zval_list_to_php_array_list(return_value, intern->array.circular_buffer, intern->array.size) for deque.
-	const size_t len = intern->array.size;
-	if (!len) {
-		RETURN_EMPTY_ARRAY();
-	}
-	ZEND_ASSERT(intern->array.mask > 0);
-
-	zend_array *values = zend_new_array(len);
-	/* Initialize return array */
-	zend_hash_real_init_packed(values);
-
-	zval *const from_buffer_start = intern->array.circular_buffer;
-	zval *from_begin = &from_buffer_start[intern->array.offset];
-	zval *const from_end = &from_buffer_start[intern->array.mask + 1];
-	ZEND_ASSERT(from_begin <= from_end);
-	/* Go through values and add values to the return array */
-	ZEND_HASH_FILL_PACKED(values) {
-		for (size_t i = 0; i < len; i++) {
-			if (from_begin == from_end) {
-				from_begin = from_buffer_start;
-			}
-			Z_TRY_ADDREF_P(from_begin);
-			ZEND_HASH_FILL_ADD(from_begin);
-			from_begin++;
-		}
-	} ZEND_HASH_FILL_END();
-	RETURN_ARR(values);
-}
-
-PHP_METHOD(Collections_Deque, jsonSerialize)
-{
-	/* json_encoder.c will always encode objects as {"0":..., "1":...}, and detects recursion if an object returns its internal property array, so we have to return a new array */
-	ZEND_PARSE_PARAMETERS_NONE();
-	collections_deque *intern = Z_DEQUE_P(ZEND_THIS);
-	collections_deque_return_list(return_value, intern);
-}
-
 PHP_MINIT_FUNCTION(collections_deque)
 {
 	collections_ce_Deque = register_class_Collections_Deque(zend_ce_aggregate, zend_ce_countable, php_json_serializable_ce, zend_ce_arrayaccess);
diff --git a/ext/collections/collections_deque.stub.php b/ext/collections/collections_deque.stub.php
@@ -13,20 +13,21 @@
  *
  * See https://en.wikipedia.org/wiki/Double-ended_queue
  *
- * This supports amortized constant time pushing and popping onto the front or back of the array.
+ * This supports amortized constant time pushing and popping onto the start (i.e. start, first)
+ * or back (i.e. end, last) of the Deque.
  *
- * Naming is based on https://www.php.net/spldoublylinkedlist
- * and on array_push/pop/unshift/shift.
+ * Method naming is based on https://www.php.net/spldoublylinkedlist
+ * and on array_push/pop/unshift/shift/ and array_key_first/array_key_last.
  */
 final class Deque implements IteratorAggregate, Countable, JsonSerializable, ArrayAccess
 {
     /** Construct the Deque from the values of the Traversable/array, ignoring keys */
     public function __construct(iterable $iterator = []) {}
     /**
-     * Returns an iterator that accounts for calls to shift/unshift tracking the position of the front of the Deque.
+     * Returns an iterator that accounts for calls to shift/unshift tracking the position of the start of the Deque.
      * Calls to shift/unshift will do the following:
      * - Increase/Decrease the value returned by the iterator's key()
-     *   by the number of elements added/removed to/from the front of the Deque.
+     *   by the number of elements added/removed to/from the start of the Deque.
      *   (`$deque[$iteratorKey] === $iteratorValue` at the time the key and value are returned).
      * - Repeated calls to shift will cause valid() to return false if the iterator's
      *   position ends up before the start of the Deque at the time iteration resumes.
@@ -40,6 +41,7 @@ public function isEmpty(): bool {}
     /** Removes all elements from the Deque. */
     public function clear(): void {}
 
+    /** @implementation-alias Collections\Deque::toArray */
     public function __serialize(): array {}
     public function __unserialize(array $data): void {}
     /** Construct the Deque from the values of the array, ignoring keys */
@@ -51,27 +53,35 @@ public function push(mixed ...$values): void {}
     public function unshift(mixed ...$values): void {}
     /**
      * Pops a value from the end of the Deque.
-     * @throws UnderflowException if the Deque is empty
+     * @throws \UnderflowException if the Deque is empty
      */
     public function pop(): mixed {}
     /**
-     * Shifts a value from the front of the Deque.
-     * @throws UnderflowException if the Deque is empty
+     * Shifts a value from the start of the Deque.
+     * @throws \UnderflowException if the Deque is empty
      */
     public function shift(): mixed {}
 
-    /** Peeks at the value at the start of the Deque, throws if empty */
-    public function bottom(): mixed {}
-    /** Peeks at the value at the end of the Deque, throws if empty */
-    public function top(): mixed {}
+    /**
+     * Peeks at the value at the start of the Deque.
+     * @throws \UnderflowException if the Deque is empty
+     */
+    public function first(): mixed {}
+    /**
+     * Peeks at the value at the end of the Deque.
+     * @throws \UnderflowException if the Deque is empty
+     */
+    public function last(): mixed {}
 
-    /** Returns a list of the elements from front to back. */
+    /**
+     * Returns a list of the elements from the start to the end.
+     */
     public function toArray(): array {}
 
     // Must be mixed for compatibility with ArrayAccess
     /**
      * Returns the value at offset (int)$offset (relative to the start of the Deque)
-     * @throws OutOfBoundsException if the value of (int)$offset is not within the bounds of this vector
+     * @throws \OutOfBoundsException if the value of (int)$offset is not within the bounds of this vector
      */
     public function offsetGet(mixed $offset): mixed {}
     /**
@@ -80,15 +90,17 @@ public function offsetGet(mixed $offset): mixed {}
     public function offsetExists(mixed $offset): bool {}
     /**
      * Sets the value at offset $offset (relative to the start of the Deque) to $value
-     * @throws OutOfBoundsException if the value of (int)$offset is not within the bounds of this vector
+     * @throws \OutOfBoundsException if the value of (int)$offset is not within the bounds of this vector
      */
     public function offsetSet(mixed $offset, mixed $value): void {}
-    /** Throws unconditionally */
     /**
-     * @throws RuntimeException unconditionally because unset and null are different things, unlike SplFixedArray
+     * @throws \RuntimeException unconditionally because unset and null are different things, unlike SplFixedArray
      */
     public function offsetUnset(mixed $offset): void {}
 
-    /** This is JSON serialized as a JSON array with elements from front to back */
+    /**
+     * This is JSON serialized as a JSON array with elements from the start to the end.
+     * @implementation-alias Collections\Deque::toArray
+     */
     public function jsonSerialize(): array {}
 }
diff --git a/ext/collections/collections_deque_arginfo.h b/ext/collections/collections_deque_arginfo.h
diff --git a/ext/collections/tests/Deque/top.phpt b/ext/collections/tests/Deque/top.phpt
@@ -1,5 +1,5 @@
 --TEST--
-Collections\Deque top()/bottom()
+Collections\Deque last()/first()
 --FILE--
 <?php
 
@@ -13,32 +13,32 @@ function expect_throws(Closure $cb): void {
 }
 
 $it = new Collections\Deque();
-expect_throws(fn () => $it->bottom());
-expect_throws(fn () => $it->top());
+expect_throws(fn () => $it->first());
+expect_throws(fn () => $it->last());
 for ($i = 0; $i <= 3; $i++) {
-    $it->push("top$i");
-    $it->unshift("bottom$i");
+    $it->push("last$i");
+    $it->unshift("first$i");
 }
-var_dump($it->bottom());
-var_dump($it->top());
+echo $it->first(), "\n";
+echo $it->last(), "\n";
 echo "Removing elements\n";
-var_dump($it->shift());
-var_dump($it->pop());
+echo $it->shift(), "\n";
+echo $it->pop(), "\n";
 echo "Inspecting elements after removal\n";
-var_dump($it->bottom());
-var_dump($it->top());
+echo $it->first(), "\n";
+echo $it->last(), "\n";
 printf("count=%d values=%s\n", $it->count(), json_encode($it));
 
 ?>
 --EXPECT--
-Caught UnderflowException: Cannot read bottom of empty Collections\Deque
-Caught UnderflowException: Cannot read top of empty Collections\Deque
-string(7) "bottom3"
-string(4) "top3"
+Caught UnderflowException: Cannot read first value of empty Collections\Deque
+Caught UnderflowException: Cannot read last value of empty Collections\Deque
+first3
+last3
 Removing elements
-string(7) "bottom3"
-string(4) "top3"
+first3
+last3
 Inspecting elements after removal
-string(7) "bottom2"
-string(4) "top2"
-count=6 values=["bottom2","bottom1","bottom0","top0","top1","top2"]
+first2
+last2
+count=6 values=["first2","first1","first0","last0","last1","last2"]
