-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Warn when coercing NAN to other types #19573
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d43ae9a
to
bfb238b
Compare
8da693f
to
729488f
Compare
AWS x86_64 (c7i.24xl)
Laravel 12.2.0 demo app - 100 consecutive runs, 50 warmups, 100 requests (sec)
Symfony 2.7.0 demo app - 100 consecutive runs, 50 warmups, 100 requests (sec)
Wordpress 6.2 main page - 100 consecutive runs, 20 warmups, 20 requests (sec)
bench.php - 100 consecutive runs, 10 warmups, 2 requests (sec)
|
Zend/Optimizer/block_pass.c
Outdated
* Those optimizations are not safe if the other operand end up being NAN | ||
* as BOOL/BOOL_NOT will warn which IS_EQUAL/IS_NOT_EQUAL do not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Triggered a benchmark to check the effect of this: valgrind shows no regression (changes are under 0.0x%).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do wonder if this is not actually a problem for switch cases too now that I think of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be worth it to check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it seems I completely broke switch statements...
<?php
$nan = fdiv(0, 0);
switch ($nan) {
case true:
echo "true";
break;
case false:
echo "false";
break;
}
?>
Returns nothing now, even without opcache :|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this is important for canonicalization.
37c74ce
to
7c29b7f
Compare
Zend/Optimizer/block_pass.c
Outdated
* Those optimizations are not safe if the other operand end up being NAN | ||
* as BOOL/BOOL_NOT will warn which IS_EQUAL/IS_NOT_EQUAL do not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be worth it to check
7c29b7f
to
895b751
Compare
6978b51
to
1640beb
Compare
1640beb
to
d82cfee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't see anything obviously incorrect.
Zend/Optimizer/block_pass.c
Outdated
* Those optimizations are not safe if the other operand end up being NAN | ||
* as BOOL/BOOL_NOT will warn which IS_EQUAL/IS_NOT_EQUAL do not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this is important for canonicalization.
Co-authored-by: Ilija Tovilo <ilija.tovilo@me.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No RM objections 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did look in particular detail. Not happy about the optimizations we lose 😔
IS_EQUAL does not warn, whereas BOOL/BOOL_NOT do now
was this change intentionally only implemented for NAN, but not for INF? it seems INF has similar cast semantics: https://3v4l.org/lYIng |
Yes, that's normal. NAN is meant to be "uncomparable" and only occurs in "nonsensical" FP operations, +-INF is just a very large value, however if you try to cast them to INT you would get another warning (done in a separate PR). |
This PR was merged into the 6.4 branch. Discussion ---------- do not coerce NAN to other types | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | | License | MIT see https://wiki.php.net/rfc/warnings-php-8-5#coercing_nan_to_other_types and php/php-src#19573 Commits ------- cd93939 do not coerce NAN to other types
RFC: https://wiki.php.net/rfc/warnings-php-8-5#coercing_nan_to_other_types