Add SplFixedArray::__set_state #6261
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -195,23 +195,18 @@ static HashTable* spl_fixedarray_object_get_gc(zend_object *obj, zval **table, i | |||||||
| return ht; | ||||||||
| } | ||||||||
|
|
||||||||
| static HashTable* spl_fixedarray_get_properties_for(zend_object *obj, zend_prop_purpose purpose) | ||||||||
| { | ||||||||
| spl_fixedarray_object *intern = spl_fixed_array_from_obj(obj); | ||||||||
|
|
||||||||
| /* Keep the values and properties separate*/ | ||||||||
| HashTable *ht = zend_array_dup(zend_std_get_properties(obj)); | ||||||||
|
|
||||||||
| if (!spl_fixedarray_empty(&intern->array)) { | ||||||||
| for (zend_long i = 0; i < intern->array.size; i++) { | ||||||||
| zend_hash_index_add_new(ht, i, &intern->array.elements[i]); | ||||||||
|
There was a problem hiding this comment. This looks correct - If I remember correctly, everything in zend_std_get_properties is a string starting in php 7.x(I forget the exact minor version), and even 123 gets coerced to php > $x = new SplFixedArray(1); $x[0] = 'field'; $x->{'0'} = 'prop'; var_dump($x);
object(SplFixedArray)#1 (2) {
["0"]=>
string(4) "prop"
[0]=>
string(5) "field"
}
php > echo serialize($x);
O:13:"SplFixedArray":2:{s:1:"0";s:4:"prop";i:0;s:5:"field";} |
||||||||
| Z_TRY_ADDREF(intern->array.elements[i]); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| return ht; | ||||||||
|
|
@@ -628,7 +623,6 @@ PHP_METHOD(SplFixedArray, fromArray) | |||||||
| } else if (num > 0 && !save_indexes) { | ||||||||
| zval *element; | ||||||||
| zend_long i = 0; | ||||||||
| spl_fixedarray_init(&array, num); | ||||||||
|
|
||||||||
| ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(data), element) { | ||||||||
|
|
@@ -754,6 +748,120 @@ PHP_METHOD(SplFixedArray, getIterator) | |||||||
| zend_create_internal_iterator_zval(return_value, ZEND_THIS); | ||||||||
| } | ||||||||
|
|
||||||||
| /* Assumes the object has been created and the spl_fixedarray_object's | ||||||||
| * array member is uninitialized or zero-initialized. | ||||||||
| * The state can have string keys, but they must come prior to integers: | ||||||||
| * | ||||||||
| * SplFixedArray::__set_state(array( | ||||||||
| * 'property' => 'value', | ||||||||
| * 0 => 1, | ||||||||
| * 1 => 2, | ||||||||
| * 2 => 3, | ||||||||
| * )) | ||||||||
| */ | ||||||||
| static zend_result spl_fixedarray_import(zend_object *object, HashTable *ht) | ||||||||
| { | ||||||||
| spl_fixedarray_object *intern = spl_fixed_array_from_obj(object); | ||||||||
| spl_fixedarray *fixed = &intern->array; | ||||||||
|
|
||||||||
| spl_fixedarray_init(fixed, 0); | ||||||||
|
|
||||||||
| /* For performance we do a single pass over the input array and the | ||||||||
| * spl_fixedarray elems. This complicates the implementation, but part | ||||||||
| * of the reason for choosing SplFixedArray is for peformance, and | ||||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||||
| * although that is primarily for memory we should still care about CPU. | ||||||||
| * | ||||||||
| * We need to track the number of string keys, which must come before | ||||||||
| * all the integer keys. This way the total size of the input minus the | ||||||||
| * number of string keys equals the number of integer keys, so we can | ||||||||
| * allocate the spl_fixedarray.elements and do this in a single pass. | ||||||||
| */ | ||||||||
|
|
||||||||
| // The total size of the input array | ||||||||
| const zend_long nht = zend_hash_num_elements(ht); | ||||||||
| zend_long nstrings = 0; // The number of string keys | ||||||||
| zend_long nints; // will be nht - nstrings | ||||||||
|
|
||||||||
| zend_string *str_idx = NULL; // current string key of input array | ||||||||
| zend_ulong num_idx = 0; // current int key (valid iff str_idx == NULL) | ||||||||
| zval *val = NULL; // current value of input array | ||||||||
| zend_long max_idx = -1; // the largest index found so far | ||||||||
| zval *begin = NULL; // pointer to beginning of fixedarray's storage | ||||||||
| zval *end = NULL; // points one element passed the last element | ||||||||
| const char *ex_msg = NULL; // message for the value exception | ||||||||
|
|
||||||||
| ZEND_HASH_FOREACH_KEY_VAL(ht, num_idx, str_idx, val) { | ||||||||
|
There was a problem hiding this comment.
Suggested change
This should either dereference references or throw an exception, e.g. for |
||||||||
| if (str_idx != NULL) { | ||||||||
|
There was a problem hiding this comment. Would UNEXPECTED make sense - common use cases wouldn't add dynamic properties |
||||||||
| if (UNEXPECTED(max_idx >= 0)) { | ||||||||
| ex_msg = "must have all its string keys come before all integer keys"; | ||||||||
| goto value_error; | ||||||||
| } | ||||||||
|
|
||||||||
| ++nstrings; | ||||||||
| object->handlers->write_property(object, str_idx, val, NULL); | ||||||||
|
|
||||||||
| } else if (UNEXPECTED((zend_long)num_idx != ++max_idx)) { | ||||||||
| ex_msg = "did not have integer keys that start at 0 and increment sequentially"; | ||||||||
| goto value_error; | ||||||||
|
|
||||||||
| } else { | ||||||||
| if (UNEXPECTED(max_idx == 0)) { | ||||||||
|
There was a problem hiding this comment. nit: UNEXPECTED may be a bit worse for performance - it's guaranteed to happen exactly once in non-empty SplFixedArrays, and the typical fixed array is small. It moves the code to a cold region far away from the fast code. The UNEXPECTED macro means you think there's a 90%+ chance the branch won't be taken in gcc by default https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html
|
||||||||
| nints = nht - nstrings; | ||||||||
|
There was a problem hiding this comment. I think this needs to check if the size changed to be safe? If there's UNEXPECTEDly a non-empty array when this line is changed, then either
For example, really ill-formed code might do the following - this may be worth adding as a unit test (does it warn about a leak with --enable-debug) class MyFixedArray extends SplFixedArray {
public function __set($name, $value) { $this->setSize(1); $this[0] = [$name, $value]; }
}
// write_property calls __set?
MyFixedArray::__set_state(['key' => 'value', 0 => 'other value']); |
||||||||
| fixed->size = nints; | ||||||||
| fixed->elements = | ||||||||
| safe_emalloc(nints, sizeof(zval), 0); | ||||||||
| begin = fixed->elements; | ||||||||
| end = fixed->elements + nints; | ||||||||
| } | ||||||||
|
|
||||||||
| ZEND_ASSERT(num_idx == max_idx); | ||||||||
|
There was a problem hiding this comment. this is redundant from the previous condition |
||||||||
| ZEND_ASSERT(begin != end); | ||||||||
|
|
||||||||
| ZVAL_COPY(begin++, val); | ||||||||
| } | ||||||||
| } ZEND_HASH_FOREACH_END(); | ||||||||
|
|
||||||||
| ZEND_ASSERT(begin == end); | ||||||||
| return SUCCESS; | ||||||||
|
|
||||||||
| value_error: | ||||||||
| spl_fixedarray_dtor_range(fixed, 0, max_idx); | ||||||||
| if (fixed->elements) { | ||||||||
| efree(fixed->elements); | ||||||||
| } | ||||||||
|
|
||||||||
| /* Zero-initialize so the object release is valid. */ | ||||||||
| spl_fixedarray_init(fixed, 0); | ||||||||
| zend_object_release(object); | ||||||||
|
|
||||||||
| zend_argument_value_error(1, ex_msg); | ||||||||
| return FAILURE; | ||||||||
| } | ||||||||
|
|
||||||||
| PHP_METHOD(SplFixedArray, __set_state) | ||||||||
| { | ||||||||
| zval *state = NULL; | ||||||||
|
|
||||||||
| ZEND_PARSE_PARAMETERS_START(1, 1) | ||||||||
| Z_PARAM_ARRAY(state); | ||||||||
| ZEND_PARSE_PARAMETERS_END(); | ||||||||
|
|
||||||||
| HashTable *ht = Z_ARRVAL_P(state); | ||||||||
| zend_class_entry *called_scope = zend_get_called_scope(execute_data); | ||||||||
|
|
||||||||
| zend_result result = object_init_ex(return_value, called_scope); | ||||||||
| if (UNEXPECTED(result != SUCCESS)) { | ||||||||
| ZVAL_NULL(return_value); | ||||||||
| RETURN_THROWS(); | ||||||||
| } | ||||||||
|
|
||||||||
| zend_object *object = Z_OBJ_P(return_value); | ||||||||
| if (UNEXPECTED(spl_fixedarray_import(object, ht) != SUCCESS)) { | ||||||||
| ZVAL_NULL(return_value); | ||||||||
| RETURN_THROWS(); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| static void spl_fixedarray_it_dtor(zend_object_iterator *iter) | ||||||||
| { | ||||||||
| zval_ptr_dtor(&iter->data); | ||||||||
|
|
@@ -844,7 +952,8 @@ PHP_MINIT_FUNCTION(spl_fixedarray) | |||||||
| spl_handler_SplFixedArray.unset_dimension = spl_fixedarray_object_unset_dimension; | ||||||||
| spl_handler_SplFixedArray.has_dimension = spl_fixedarray_object_has_dimension; | ||||||||
| spl_handler_SplFixedArray.count_elements = spl_fixedarray_object_count_elements; | ||||||||
| spl_handler_SplFixedArray.get_properties_for | ||||||||
| = spl_fixedarray_get_properties_for; | ||||||||
| spl_handler_SplFixedArray.get_gc = spl_fixedarray_object_get_gc; | ||||||||
| spl_handler_SplFixedArray.dtor_obj = zend_objects_destroy_object; | ||||||||
| spl_handler_SplFixedArray.free_obj = spl_fixedarray_object_free_storage; | ||||||||
|
|
||||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| --TEST-- | ||
| SplFixedArray: __set_state | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| $fixed = SplFixedArray::__set_state([1, 2, 3]); | ||
| var_export($fixed); | ||
| echo "\n---\n"; | ||
|
|
||
| // dynamic properties | ||
| $fixed->undefined = 'undef'; | ||
| var_export($fixed); | ||
| echo "\n---\n"; | ||
|
|
||
| // inheritance | ||
| class Vec | ||
| extends SplFixedArray | ||
| { | ||
| public $type; | ||
| } | ||
| $vec = new Vec(3); | ||
| $vec[0] = 1; | ||
| $vec[1] = 2; | ||
| $vec[2] = 3; | ||
| $vec->type = 'int'; | ||
| var_export($vec); | ||
| echo "\n---\n"; | ||
|
|
||
| $vec->undefined = 'undef'; | ||
| var_export($vec); | ||
|
|
||
| ?> | ||
| --EXPECT-- | ||
| SplFixedArray::__set_state(array( | ||
| 0 => 1, | ||
| 1 => 2, | ||
| 2 => 3, | ||
| )) | ||
| --- | ||
| SplFixedArray::__set_state(array( | ||
| 'undefined' => 'undef', | ||
| 0 => 1, | ||
| 1 => 2, | ||
| 2 => 3, | ||
| )) | ||
| --- | ||
| Vec::__set_state(array( | ||
| 'type' => 'int', | ||
| 0 => 1, | ||
| 1 => 2, | ||
| 2 => 3, | ||
| )) | ||
| --- | ||
| Vec::__set_state(array( | ||
| 'type' => 'int', | ||
| 'undefined' => 'undef', | ||
| 0 => 1, | ||
| 1 => 2, | ||
| 2 => 3, | ||
| )) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| --TEST-- | ||
| SplFixedArray: __set_state | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| try { | ||
| $fixed = SplFixedArray::__set_state([1 => 2]); | ||
| } catch (ValueError $error) { | ||
| var_dump($error->getMessage()); | ||
| } | ||
|
|
||
| try { | ||
| $fixed = SplFixedArray::__set_state([0 => 1, 1 => 2, 3 => 4]); | ||
| } catch (ValueError $error) { | ||
| var_dump($error->getMessage()); | ||
| } | ||
|
|
||
| try { | ||
| $fixed = SplFixedArray::__set_state([-1 => 2]); | ||
| } catch (ValueError $error) { | ||
| var_dump($error->getMessage()); | ||
| } | ||
|
|
||
| echo "-----\n"; | ||
|
|
||
| try { | ||
| $fixed = SplFixedArray::__set_state([0 => 1, 'type' => 'undefined']); | ||
| } catch (ValueError $error) { | ||
| var_dump($error->getMessage()); | ||
| } | ||
| --EXPECTF-- | ||
| string(%d) "SplFixedArray::__set_state(): Argument #1 ($state) did not have integer keys that start at 0 and increment sequentially" | ||
| string(%d) "SplFixedArray::__set_state(): Argument #1 ($state) did not have integer keys that start at 0 and increment sequentially" | ||
| string(%d) "SplFixedArray::__set_state(): Argument #1 ($state) did not have integer keys that start at 0 and increment sequentially" | ||
| ----- | ||
| string(%d) "SplFixedArray::__set_state(): Argument #1 ($state) must have all its string keys come before all integer keys" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| --TEST-- | ||
| SplFixedArray: __set_state export and import | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| function compare_export($var, string $case) { | ||
| $exported = var_export($var, true); | ||
|
|
||
| $imported = eval("return {$exported};"); | ||
| $reexported = var_export($imported, true); | ||
|
|
||
| if ($exported == $reexported) { | ||
| echo "Pass {$case}.\n"; | ||
| } else { | ||
| echo "Fail {$case}.\nExpected:\n{$exported}\nActual:\n{$reexported}\n"; | ||
| } | ||
| } | ||
|
|
||
| // simple | ||
| $fixed = SplFixedArray::__set_state([1, 2, 3]); | ||
| compare_export($fixed, 'simple'); | ||
|
|
||
| // dynamic properties | ||
| $fixed->undefined = 'undef'; | ||
| compare_export($fixed, 'dynamic properties'); | ||
|
|
||
| // inheritance | ||
| class Vec | ||
| extends SplFixedArray | ||
| { | ||
| public $type; | ||
| } | ||
| $vec = new Vec(3); | ||
| $vec[0] = 1; | ||
| $vec[1] = 2; | ||
| $vec[2] = 3; | ||
| $vec->type = 'int'; | ||
|
|
||
| compare_export($vec, 'inheritance'); | ||
|
|
||
| // dynamic properties and inheritance | ||
| $vec->undefined = 'undef'; | ||
| compare_export($vec, 'dynamic properties and inheritance'); | ||
|
|
||
| --EXPECT-- | ||
| Pass simple. | ||
| Pass dynamic properties. | ||
| Pass inheritance. | ||
| Pass dynamic properties and inheritance. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#8046
I was working with
get_properties_foragain after the last review, I'm pretty sure this will cause new problems with var_export onget_properties_forif #8046 isn't merged, for