tree-wide: Refactor with zend_string_ends_with* - #23216
Conversation
jorgsowa
left a comment
jorgsowa
left a comment
There was a problem hiding this comment.
Looks good for ext/session
| return filename && ZSTR_LEN(filename) > sizeof(".phar") - 1 && | ||
| zend_string_ends_with_literal(filename, ".phar") && |
There was a problem hiding this comment.
You no longer need the ZSTR_LEN comparison, as that's taken care of by zend_string_ends_with_literal
There was a problem hiding this comment.
Yes this is correct!
|
|
||
| int result; | ||
| if (ZSTR_LEN(tmp) > 0 && ZSTR_VAL(tmp)[ZSTR_LEN(tmp) - 1] != '\n') { | ||
| if (ZSTR_LEN(tmp) > 0 && !zend_string_ends_with_literal(tmp, "\n")) { |
There was a problem hiding this comment.
This has a behavior impact when you delete the ZSTR_LEN(tmp) > 0 check.
You see, when tmp is an empty string, here !zend_string_ends_with_literal(tmp, "\n") is true, and we clearly don't want to add \n in empty strings.
| } else { | ||
| return false; | ||
| } | ||
| return ZSTR_LEN(host) > ZSTR_LEN(domain) && zend_string_ends_with(host, domain); |
There was a problem hiding this comment.
I think this shouldn't be changed. This missed the ZSTR_LEN(host) == ZSTR_LEN(domain) case.
| } | ||
|
|
||
| bool empty = ZSTR_VAL(buf->s)[ZSTR_LEN(buf->s) - 1] != ','; | ||
| bool empty = !zend_string_ends_with_literal(buf->s, ","); |
There was a problem hiding this comment.
Are these calls inlined? As this might have a performance impact? (Mainly asking as I know some work was done to improve the performance of the encoder recently)
There was a problem hiding this comment.
They are inlined (zend_always_inline). The only performance diff is the function has a ZSTR_LEN(str) >= 1 check (in this case), which is faster in some cases and slower in some cases
There was a problem hiding this comment.
In JSON cases, the buffer has ensure the strings can't be empty strings. So comparing using raw bytes is faster here :)
I think this is the only case in the PR.
5 participants
Utilize
zend_string_ends_with*APIs added in #22819.