Array_get and array_has functions#21637
Conversation
ext/standard/array.c
Outdated
|
|
||
| if (dot == NULL) { | ||
| /* No dot found, this is a simple key lookup */ | ||
| zend_string *zkey = zend_string_init(key, key_len, 0); |
There was a problem hiding this comment.
If you have a zend_string already here, then you can avoid the allocation.
In general, use zend_symtable_str_find (also below).
ext/standard/array.c
Outdated
|
|
||
| /* If key is null, return the whole array */ | ||
| if (key == NULL || Z_TYPE_P(key) == IS_NULL) { | ||
| ZVAL_ARR(return_value, zend_array_dup(ht)); |
There was a problem hiding this comment.
Avoid a duplication, just return a copy with the refcount. Sadly, ZVAL_ARR sucks as it doesn't take into account the mutability in the type info; so you'd have to use Z_PARAM_ARRAY so you have a zval that you can use RETURN_COPY on.
ext/standard/array.c
Outdated
| result = array_get_nested(ht, Z_STRVAL_P(key), Z_STRLEN_P(key)); | ||
|
|
||
| if (result != NULL) { | ||
| ZVAL_COPY(return_value, result); |
ext/standard/array.c
Outdated
| result = zend_hash_index_find(ht, Z_LVAL_P(key)); | ||
|
|
||
| if (result != NULL) { | ||
| ZVAL_COPY(return_value, result); |
ext/standard/array.c
Outdated
|
|
||
| /* Key not found, return default value */ | ||
| if (default_value != NULL) { | ||
| ZVAL_COPY(return_value, default_value); |
ext/standard/array.c
Outdated
| if (default_value != NULL) { | ||
| ZVAL_COPY(return_value, default_value); | ||
| } else { | ||
| RETVAL_NULL(); |
There was a problem hiding this comment.
Not necessary, the return value is NULL implicitly
ext/standard/array.c
Outdated
| } | ||
|
|
||
| /* Invalid key type */ | ||
| RETURN_FALSE; |
There was a problem hiding this comment.
This case is impossible by using the type int|string, use ZEND_UNREACHABLE();
There was a problem hiding this comment.
Not yet impossible, because Z_PARAM_ZVAL, but yes, it should be properly typed in ZPP.
ext/standard/array.c
Outdated
| } | ||
|
|
||
| /* Recurse into the nested array with the remaining key */ | ||
| return array_get_nested(Z_ARRVAL_P(current), dot + 1, key_len - segment_len - 1); |
There was a problem hiding this comment.
Unless this gets tailcall eliminated, you have primitive recursion here which can overflow. Best to write an explicit loop.
ext/standard/array.c
Outdated
| current = zend_symtable_find(ht, segment); | ||
| zend_string_release(segment); | ||
|
|
||
| if (current == NULL || Z_TYPE_P(current) != IS_ARRAY) { |
There was a problem hiding this comment.
Does this handle references properly?
|
@ndossche thanks for your review. Following some comments on the RFC discussion I have updated the implementation to also accept an array in the $key parameter. I also looked into all the other issues that you raised (though some do not apply any longer) |
| } | ||
|
|
||
| /* Handle integer keys (simple lookup) */ | ||
| ZEND_ASSERT(Z_TYPE_P(key) == IS_LONG); |
There was a problem hiding this comment.
This is insufficient, the ZPP parsing still accepts any argument. The stubs don't enforce the argument type check in any way.
| /* Use php_explode to split the string by '.' */ | ||
| zend_string *delim = ZSTR_CHAR('.'); | ||
| array_init(&segments_array); | ||
| php_explode(delim, Z_STR_P(key), &segments_array, ZEND_LONG_MAX); |
There was a problem hiding this comment.
This is a bit of an inefficient way to go about it.
| result = zend_hash_index_find(ht, Z_LVAL_P(key)); | ||
|
|
||
| if (result != NULL) { | ||
| RETURN_COPY(result); |
There was a problem hiding this comment.
This won't work properly with references I believe.
The function stub doesn't seem to return a reference, so this needs to be RETURN_COPY_DEREF (same issue is also present in other places).
4 participants
This PR proposes adding two small, focused array functions to ext/standard for retrieving and checking nested array elements using dot notation, in a single step.
See the related RFC:
https://wiki.php.net/rfc/array_get_and_array_has