ext/standard/array.c: various refactoring related to extract() function #20481
Conversation
Girgias
force-pushed
the
array-refactoring-2025-11-extract
branch
from
8e98d2e to
123d9e3
Compare
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
Changes to such sensitive code make me a bit nervous ngl
| /* }}} */ | ||
|
|
||
| static zend_always_inline int php_valid_var_name(const char *var_name, size_t var_name_len) /* {{{ */ | ||
| static zend_always_inline bool php_valid_var_name(const zend_string *var_name) /* {{{ */ |
There was a problem hiding this comment.
One thing that I always kinda wonder with changing (char*, len) pairs to zend_string is how much flexibility we lose.
A pair is more flexible because you can pass plain old C strings to it, while now you'd need a zend_string which we can't efficiently make from a C string.
Granted, in this particular case it doesn't really matter but it's food for thought.
There was a problem hiding this comment.
In general, I think we should move things to zend_string* rather than a char* + size_t pair. The main reason why certain parts of the engine (e.g. SAPI or streams) use a pair is because it predates PHP 7 and the introduction of them.
But in this instance all the inputs are zend_strings anyway so this improves readability IMHO.
There was a problem hiding this comment.
I think it makes sense to use zend_string* if it's pretty much always passed there like here (all calls looks like php_valid_var_name(ZSTR_VAL(var_name), ZSTR_LEN(var_name))) but many parts of streams and SAPI are different and I would prefer not to over-use zend_string* there.
ext/standard/array.c
Outdated
| zend_string *str = zend_long_to_str(num_key); | ||
| php_prefix_varname(&final_name, prefix, ZSTR_VAL(str), ZSTR_LEN(str), true); | ||
| zend_string_release_ex(str, 0); | ||
| zend_string *str = zend_ulong_to_str(num_key); |
There was a problem hiding this comment.
Why ulong? If the key is a negative number the behaviour will differ.
I also almost missed this due to the commit title. If you were to do such a change this should be done in a separate commit.
There was a problem hiding this comment.
prefix is already a ulong so that's why I went for that changre, but yes should be in a seperate commit must have messed up when I tried rearranging order of them or something.
There was a problem hiding this comment.
Added a test, as I expected we actually drop indexes which are negative... probably should warn.
Girgias
force-pushed
the
array-refactoring-2025-11-extract
branch
from
123d9e3 to
3319bee
Compare
Girgias
force-pushed
the
array-refactoring-2025-11-extract
branch
from
3319bee to
66946a9
Compare
|
Seems like a bit of waste of time that is risky but I don't see any visible issue from the quick look. Doesn't mean that there is none... |
|
Btw. I'm not questioning that the new code isn't better. It looks like an improvement to me. Just not sure if benefit (which is really more cosmetic) outweights the risks and if the amount of work to put into this is really worth it. |
There was a problem hiding this comment.
prefix is already a ulong so that's why I went for that changre
Hash table keys are always ulongs, what matters is how they are interpreted.
Your test regarding the ulong changes is wrong. This shows a difference in behaviour:
<?php
$arr = [-1 => "foo"];
var_dump(extract($arr, EXTR_PREFIX_ALL | EXTR_REFS, "prefix"));Please revert the long->ulong changes
3 participants
Commits should be reviewed in order.