-
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.
seems the location is correct :) https://github.com/php/php-src/pull/15965/files#r1768840758
Co-authored-by: Christoph M. Becker <cmbecker69@gmx.de>
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.
Thank you! This looks good to me, but maybe adding some more test cases makes sense; escpecially giving a width in the format specifier to show the alignment, and maybe some more, e.g. using 0 padding.
And I would also change the max_width
for NaN
, but that can be done in a follow-up PR, too.
Anyhow, since we're late in the PHP 8.4.0 pre-release cycle, this requires approval from the @php/release-managers-84.
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)