Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

divinity76
Copy link
Contributor

@divinity76 divinity76 commented Sep 20, 2024

sprintf("%.17g", -INF); should return -INF not INF

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)

@cmb69
Copy link
Member

cmb69 commented Sep 20, 2024

Note that here is some overlap with #10298 (and that likely neither solution solves #9209).

@divinity76
Copy link
Contributor Author

Oh I was unaware.

Close this PR then? this is a low-effort fix~

Copy link
Member

@cmb69 cmb69 left a 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.

ext/standard/formatted_print.c Outdated Show resolved Hide resolved
ext/standard/formatted_print.c Outdated Show resolved Hide resolved
ext/standard/formatted_print.c Outdated Show resolved Hide resolved
Copy link
Member

@cmb69 cmb69 left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sprintf %.17g negative infinity bug
2 participants