-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
fix printf negative infinity #15965
base: master
Are you sure you want to change the base?
fix printf negative infinity #15965
Conversation
Oh I was unaware. Close this PR then? this is a low-effort fix~ |
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.
In my opinion, this is a better approach than #10298, since it is way less intrusive.
// ideally negative should have been handled inside php_sprintf_appendstring , | ||
// but the code is difficult to debug, and it's easy to break stuff inside there (I tried, I broke stuff), | ||
// handling it here is much easier.. |
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.
Handling this here is correct.
// ideally negative should have been handled inside php_sprintf_appendstring , | ||
// but the code is difficult to debug, and it's easy to break stuff inside there (I tried, I broke stuff), | ||
// handling it here is much easier.. | ||
php_sprintf_appendstring(buffer, pos, "-INF", 4, 0, padding, |
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.
Hard-coding the min_width
argument (4
) here, looks wrong to me (I am aware that this already had been done this way before); see https://3v4l.org/WtHcq. Why not simply pass width
instead? Same for the NaN
and the -INF
cases.
php_sprintf_appendstring(buffer, pos, "INF", 3, 0, padding, | ||
alignment, 3, is_negative, 0, always_sign); |
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.
Instead of having an if-else, we could do:
php_sprintf_appendstring(buffer, pos, "INF", 3, 0, padding, | |
alignment, 3, is_negative, 0, always_sign); | |
char *str = is_negative ? "-INF" : "INF"; | |
php_sprintf_appendstring(buffer, pos, str, strlen(str), 0, padding, | |
alignment, strlen(str), is_negative, 0, always_sign); |
sprintf("%.17g", -INF);
should return-INF
notINF
resolves #15964
I tried fixing it inside php_sprintf_appendstring as it seems the original author intended, but.. that was difficult, and I broke other stuff.
(I think php_sprintf_appendstring would benefit from using smart_str https://www.phpinternalsbook.com/php7/internal_types/strings/smart_str.html , but idk how much effort that would be)