ext/phar: avoid redundant allocation by using zend_string for alias#21785
ext/phar: avoid redundant allocation by using zend_string for alias#21785arshidkv12 wants to merge 4 commits intophp:masterfrom
Conversation
iluuu1994
left a comment
iluuu1994
left a comment
There was a problem hiding this comment.
No technical review performed.
| } | ||
|
|
||
| if (!actual_alias && zend_string_equals_literal(entry.filename, ".phar/alias.txt")) { | ||
| if (actual_alias == NULL && zend_string_equals_literal(entry.filename, ".phar/alias.txt")) { |
There was a problem hiding this comment.
Please avoid changes like this. There are others in this PR.
| mydata->is_temporary_alias = 0; | ||
|
|
||
| if (NULL != (fd_ptr = zend_hash_str_find_ptr(&(PHAR_G(phar_alias_map)), actual_alias, mydata->alias_len))) { | ||
| if (NULL != (fd_ptr = zend_hash_str_find_ptr(&(PHAR_G(phar_alias_map)), ZSTR_VAL(actual_alias), ZSTR_LEN(actual_alias)))) { |
|
@Girgias Would you like to review this? I saw you make some phar PRs, I don't know if that means you're interested in getting more familiar with it? |
|
I'm currently in the process of making the code of it simpler to understand. I usually spend some time every year to do this :) |
Girgias
left a comment
Girgias
left a comment
There was a problem hiding this comment.
I'm not really happy with the approach here. ext/phar is a complicated extensions and the "avoid reallocations" TODOs mostly involve actually modifying the structs to use zend_strings. Not just use them sporadically.
Especially as you seem to "blindly" be doing some changes without changing surrounding context that would benefit from using zend_strings.
| mydata->is_temporary_alias = 0; | ||
|
|
||
| if (NULL != (fd_ptr = zend_hash_str_find_ptr(&(PHAR_G(phar_alias_map)), actual_alias, mydata->alias_len))) { | ||
| if (NULL != (fd_ptr = zend_hash_str_find_ptr(&(PHAR_G(phar_alias_map)), ZSTR_VAL(actual_alias), ZSTR_LEN(actual_alias)))) { |
There was a problem hiding this comment.
Indent, but you have a zend_string use the HashTable API that uses zend_string...
| zend_hash_str_add_ptr(&(PHAR_G(phar_fname_map)), mydata->fname, fname_len, mydata); | ||
|
|
||
| if (actual_alias) { | ||
| if (actual_alias != NULL) { |
There was a problem hiding this comment.
Don't change this, the not equal to NULL syntax is quite confusing.
| if (actual_alias != NULL) { | |
| if (actual_alias) { |
| actual_alias = php_stream_copy_to_mem(fp, entry.uncompressed_filesize, 0); | ||
| entry.uncompressed_filesize = ZSTR_LEN(actual_alias); |
There was a problem hiding this comment.
I don't see how you are actually following the TODO comment here.
| actual_alias = php_stream_copy_to_mem(fp, entry.uncompressed_filesize, 0); | ||
| entry.uncompressed_filesize = ZSTR_LEN(actual_alias); |
There was a problem hiding this comment.
Ditto, not following the TODO comment.
| actual_alias = php_stream_copy_to_mem(fp, entry.uncompressed_filesize, 0); | ||
| entry.uncompressed_filesize = ZSTR_LEN(actual_alias); |
| if (actual_alias) { \ | ||
| zend_string_release_ex(actual_alias, 0); \ | ||
| actual_alias = NULL; \ | ||
| } \ |
| zend_string_release_ex(actual_alias, 0); | ||
| zend_hash_str_add_ptr(&(PHAR_G(phar_alias_map)), mydata->alias, mydata->alias_len, mydata); |
There was a problem hiding this comment.
Once again we have the zend_string to check the HashTable.
3 participants
No description provided.