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.
There was a problem hiding this comment.
This feels like if you have an empty string for data you shouldn't be doing anything in the first place.
There was a problem hiding this comment.
Yes this makes sense.
| } 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.
There was a problem hiding this comment.
I think the only reason why the length check existed was to make sure the pointer arithmetic didn't point prior to the string.
| } | ||
|
|
||
| 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.
|
|
||
| int result; | ||
| if (ZSTR_LEN(tmp) > 0 && ZSTR_VAL(tmp)[ZSTR_LEN(tmp) - 1] != '\n') { | ||
| if (ZSTR_LEN(tmp) == 0) { |
There was a problem hiding this comment.
The pgsql hunk makes the code worse: the new ZSTR_LEN(tmp) == 0 branch only exists because the helper returns false for empty strings, so you end up with a 3-branch if and a PQputCopyData() call identical to the existing else. I'd drop it, or fold it in as ZSTR_LEN(tmp) == 0 || zend_string_ends_with_literal(tmp, "\n").
5 participants
Utilize
zend_string_ends_with*APIs added in #22819.