Zend: Remove unused parameter from zend_dval_to_lval_cap() #21112
Conversation
github-actions
bot
added
Category: Engine
Extension: dom
Extension: tidy
ABI break
labels
iluuu1994
left a comment
iluuu1994
left a comment
There was a problem hiding this comment.
LGTM. Please document the API change in UPGRADING.INTERNALS.
Zend/zend_operators.c
Outdated
| lval = zend_dval_to_lval_cap(dval, op_str); | ||
| lval = zend_dval_to_lval_cap(dval); | ||
| if (!zend_is_long_compatible(dval, lval)) { | ||
| if (op_str == NULL) { |
There was a problem hiding this comment.
Do we still need to make a copy? I believe not?
There was a problem hiding this comment.
There's an existing copy above, including a warning. So I think yes. Technically not here, but then we need to know whether to free or not, which is likely not worth the trouble.
There was a problem hiding this comment.
I still don't understand. zend_tmp_string_release already checks whether we need to free.
zend_incompatible_string_to_long_error triggers a warning that uses op as a string, even if that invalidates op, then the warning will have already been triggered.
There was a problem hiding this comment.
i.e. you could do zend_incompatible_string_to_long_error(op_str ? op_str : Z_STR_P(op));
There was a problem hiding this comment.
Ah, that's what you mean. Would also be fine by me.
The `zend_string *s` parameter became unused after commit f754ffa (phpGH-20746) removed the `zend_oob_string_to_long_error()` calls. This fixes an unused-parameter compiler warning and updates a stale comment in zend_operators.c that incorrectly stated this function can emit warnings.
3 participants
The
zend_string *sparameter became unused after commit f754ffa (GH-20746) removed thezend_oob_string_to_long_error()calls.This fixes an unused-parameter compiler warning and updates a stale comment in zend_operators.c that incorrectly stated this function can emit warnings.