-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Reimplement printf library #10507
Reimplement printf library #10507
Conversation
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 took me a second to realize that the big deal here is that this fully supports localization; really stellar job as this is a much underserved niche in the rust ecosystem which has just largely ignored locale support altogether.
I actually spent a good bit of time looking at and playing with the locale code, as it's just easily the most interesting part about this in terms of what this buys us over the usual std::fmt::Display
stuff. The grouping logic can be simplified by using a conditionally repeating iterator, and it would be cool if Locale::grouping
were a bit less C-like with its logic, but it's really nice how succinctly weird or convoluted locale conventions can be expressed.
I'm still not sure how useful/useable actual printf notation is in the rust world, especially with the fact that the prefixes no longer actually control the size of the reads from the passed-in variables (thank God!). It really would be nice if there were some sort of simple locale-awareness shim/adapter we could use so that we can use native rust fmt stuff, because really the only place we actually need printf support is just the printf
builtin (unless I'm missing something?). In that sense, I wonder if this new crate could be a foundational building block; taking the logic and tests from musl (nice job with that bug report, btw!) and using printf as the "formal notation" but not actually exposing any of it for use. Once we migrate to UTF-8 for internal strings, there'll be very little incentive to not directly use the native rust functionality for as much as we can (and the same shim/adapter that supports locale awareness can handle non-unicode characters).
This adds a crate containing a new implementation of printf, ported from musl. This has some advantages: - locale support is direct instead of being "applied after". - No dependencies on libc printf. No unsafe code at all. - No more WideWrite - just uses std::fmt::Write. - Rounding is handled directly in all cases, instead of relying on Rust and/or libc. - No essential dependency on WString. - Supports %n. - Implementation is more likely to be correct since it's based on a widely used printf, instead of a low-traffic Rust crate. - Significantly faster.
Allows running printf tests.
This drops our usage of printf-compat.
The new printf is derived from musl libc. Add it to license.rst to reflect our usage.
5f4814a
to
ae87cde
Compare
We need printf support for builtin printf, and also for localized strings as you say - Rust's format strings are required to be compile-time, and the available crates for runtime formatting don't look so great but maybe one will do. For other non-localized stuff it's fine to move from printf to native Rust formatting, though there's no urgency for that either. |
We currently mostly (only?) use very simple features - our format strings are mostly
I would prefer if we had a story for localized stuff first, so we don't end up switching formatting languages constantly. Anyway, the PR at hand sounds like a good idea to me. I had a quick test and didn't find anything immediately wrong with it. |
Description
This reimplements printf (the library part, not the builtin), allowing us to drop the gnarly
printf-compat
crate.The new
printf
crate lives at top level, in the fish workspace. fish itself only required minor changes to adopt it.This is probably not feasible to review in detail, but printf-compat wasn't really reviewed either, so we're no worse off.
Design
The new printf is based on musl - porting it from C to Rust, providing longer names and comments, and adding locale support. This is an improvement because the musl printf is widely used, and
printf-compat
is not, so it is more likely to be standards conforming.There are other improvements:
printf
to anything that usesstd::fmt::Write
such asString
,WString
, or a file - no moreWideWrite
.%n
(printf-compat did not).Testing
This includes all of the printf-compat tests AND all of the musl tests. Naturally it passes all of them. This is substantially more test coverage than printf-compat has.
Fun Fact
This uncovered a musl printf buf, which got fixed!