ext/phar: Use HASH_FOREACH macros and other refactorings #16073
Conversation
ext/phar/dirstream.c
Outdated
| if (ZSTR_LEN(str_key) > path_len && | ||
| memcmp(ZSTR_VAL(str_key), ZSTR_VAL(resource->path)+1, path_len) == 0 && | ||
| IS_SLASH(ZSTR_VAL(str_key)[path_len])) { | ||
| ZEND_HASH_FOREACH_STR_KEY(&phar->manifest, str_key) { |
There was a problem hiding this comment.
Can't this be ZEND_HASH_MAP_FOREACH_STR_KEY ? I see there's already other code that assumes it's a map type array, which makes sense to me as well.
There was a problem hiding this comment.
Just gonna say also: You may only use this if you know 100% sure it's not a packed array
There was a problem hiding this comment.
Well, it seems to be passing all the tests locally, and I don't see how it could be packed.
ext/phar/dirstream.c
Outdated
| IS_SLASH(ZSTR_VAL(str_key)[path_len])) { | ||
| } ZEND_HASH_FOREACH_END(); | ||
|
|
||
| ZEND_HASH_FOREACH_STR_KEY(&phar->virtual_dirs, str_key) { |
ext/phar/dirstream.c
Outdated
|
|
||
| if (SUCCESS != zend_hash_move_forward(&phar->manifest)) { | ||
| break; | ||
| ZEND_HASH_FOREACH_STR_KEY(&phar->manifest, str_key) { |
There was a problem hiding this comment.
Same ZEND_HASH_MAP_FOREACH_STR_KEY remark
ext/phar/dirstream.c
Outdated
| break; | ||
| } | ||
| zend_string *str_key; | ||
| ZEND_HASH_FOREACH_STR_KEY(manifest, str_key) { |
There was a problem hiding this comment.
Same ZEND_HASH_MAP_FOREACH_STR_KEY remark
ext/phar/dirstream.c
Outdated
| } | ||
| entry = (char *) safe_emalloc(keylen, 1, 1); |
There was a problem hiding this comment.
You don't need to cast this to char *.
There was a problem hiding this comment.
This was copied and pasted from the original code but will remove.
ext/phar/dirstream.c
Outdated
| @@ -260,17 +223,22 @@ static php_stream *phar_make_dirstream(char *dir, HashTable *manifest) /* {{{ */ | |||
| entry[keylen - dirlen - 1] = '\0'; | |||
| keylen = keylen - dirlen - 1; | |||
| } | |||
| PHAR_ADD_ENTRY: | |||
| PHAR_ADD_ENTRY: | |||
There was a problem hiding this comment.
We should make a rule for labels, AFAIK Tim you and I all indent differently.
There was a problem hiding this comment.
To be fair, this was CLion autocorrecting it. But yes a rule would be good.
Also simplify the inner if condition
This makes it more obvious that the host_len is that of resource->host
Also simplify the inner if condition
Also simplify and refactor the inside of the loop
…am() And also pass in the known string length
Girgias
force-pushed
the
phar-foreach
branch
from
86b7fdf to
cd64aab
Compare
|
I have fixed the commits in place and forced pushed. |
| if (keylen <= dirlen) { | ||
| if (keylen == 0 || keylen < dirlen || !strncmp(ZSTR_VAL(str_key), dir, dirlen)) { |
There was a problem hiding this comment.
I don't exactly understand what these conditions are doing, but it feels like it can be refactored to use one of the zend_string APIs
There was a problem hiding this comment.
I would have to dig deeper to understand how this works, but let's cut the PR here.
| } | ||
| entry = safe_emalloc(keylen, 1, 1); | ||
| memcpy(entry, ZSTR_VAL(str_key), keylen); | ||
| entry[keylen] = '\0'; | ||
|
|
||
| goto PHAR_ADD_ENTRY; | ||
| } else { | ||
| if (0 != memcmp(ZSTR_VAL(str_key), dir, dirlen)) { |
Commits should be reviewed individually.