Skip to content

Remove custom (v)snprintf as it's always available since C99 #5086

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

Closed
wants to merge 6 commits into from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Jan 14, 2020

I suppose we maybe could also get rid of our custom format conversion implementation format_converter() in snprintf.c (which would give us support for a and A modifier) by relying on the system one.

offset->abbr = timelib_malloc(9); /* GMT±xxxx\0 */
snprintf(offset->abbr, 9, "GMT%c%02d%02d",
offset->abbr = timelib_malloc(13); /* GMT±xxxx\0 */
snprintf(offset->abbr, 13, "GMT%c%02d%02d",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh ... can we do something against this?

Copy link
Member Author

@Girgias Girgias Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe change the format precision?

Also this is only a problem on ZTS RELEASE builds and Derick has no idea why the compiler is complaining.

@Girgias Girgias force-pushed the remove-custom-snprintf-c99 branch from a2d5c20 to 1425299 Compare January 15, 2020 13:31
@Girgias Girgias force-pushed the remove-custom-snprintf-c99 branch from 1425299 to 792cf7d Compare January 15, 2020 13:33
@nikic
Copy link
Member

nikic commented Jan 15, 2020

I'm not sure about this. The custom snprintf definitely supports some non-standard things (like %H), though possibly those are not used for snprintf in particular.

@Girgias
Copy link
Member Author

Girgias commented Jan 15, 2020

I'm not sure about this. The custom snprintf definitely supports some non-standard things (like %H), though possibly those are not used for snprintf in particular.

I'm gonna have a look through the code base with grep to see if some of the non standard formats are used and if yes how often.

Some of them, like v, can be changed to a standard one without any other changes.

I suppose some of the custom ones are for locale aware conversion?

@Girgias
Copy link
Member Author

Girgias commented Jan 15, 2020

So I ran a grep regex (v)?(a)?s(n|l)?printf\(.*%(h|H|k|v|t|j|z|Z|l|L|I).*\) to see how many functions use custom formats and I get surprisingly few results [1].

I suppose we could move everything to use standard formats and use (v)snprintf even in the other custom functions so that we get rid of our custom format converter.

[1] https://gist.github.com/Girgias/b9a2b9926190630d433c84da0ef1b002

@Girgias
Copy link
Member Author

Girgias commented Jan 15, 2020

Improved the regex to (v)?(a)?s(n|l)?printf\(.*%(\+|-| |#|0)?(\d+|\*)?(\.(\d+|\*))?(h|H|k|v|t|j|z|Z|l|L|I).*\) to cover prevision and length flags; and caught 5 more case. I've updated the gist https://gist.github.com/Girgias/b9a2b9926190630d433c84da0ef1b002/revisions

@ramsey
Copy link
Member

ramsey commented May 31, 2022

@Girgias, please rebase to resolve the conflicts.

What's the status of this PR? Can we get it ready for 8.2?

@ramsey ramsey added this to the PHP 8.2 milestone May 31, 2022
@Girgias
Copy link
Member Author

Girgias commented Jun 20, 2022

@Girgias, please rebase to resolve the conflicts.

What's the status of this PR? Can we get it ready for 8.2?

I'm not sure if extensions in the wild use snprintf() with our custom modifiers, I'll need to revisit this (if ever) later.

@Girgias Girgias closed this Jun 20, 2022
@Girgias Girgias deleted the remove-custom-snprintf-c99 branch June 20, 2022 12:13
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.

4 participants