-
Notifications
You must be signed in to change notification settings - Fork 29
Support {fmt} and std::format
#505
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
Conversation
We tried this earlier (#491), but then reverted it (#495). That was just a few short weeks ago: what changed? We learned that our old format string syntax wasn't just _hard_ to implement; it was actually [impossible]. This led naturally to a [new idea] that may not be as flexible, but actually still supports the most important use cases. Happily, this new idea is very easy to implement without using any internal implementation details of the formatting libraries. It even gives us a little (_very_ little) room for evolving more options for formatting the unit label, such as setting the fill character, although we don't currently expect to ever need or want them. I was also able to verify support for `std::format` by generating a single file, and uploading it to [godbolt]. This shows that on a compiler version and setup that includes `std::format`, Au supports it out of the box with no configuration. Overall, I feel confident enough in this solution to land it for the 0.5.0 release. Fixes #149. [impossible]: fmtlib/fmt#4508 (comment) [new idea]: fmtlib/fmt#4508 (comment) [godbolt]: https://godbolt.org/z/6jhW1jboW
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.
Pull Request Overview
This PR adds support for string formatting of Au quantities using either the {fmt} library or C++20's std::format. The implementation provides a template-based formatter that allows customization of both the numeric value and unit label formatting.
- Adds
QuantityFormattertemplate to handle formatting with configurable numeric and unit label options - Provides automatic
std::formatsupport and instructions for {fmt} library integration - Includes comprehensive documentation with examples and setup instructions
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/reference/index.md | Adds reference to new format support documentation |
| docs/reference/format.md | Complete documentation for format support with examples and setup |
| au/quantity.hh | Implements QuantityFormatter template and std::format specialization |
| au/fmt_test.cc | Test suite for {fmt} library integration |
| au/BUILD.bazel | Adds build target for fmt tests |
| WORKSPACE | Adds fmt library dependency |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
chiphogg
left a comment
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.
The "zero setup" version of std::format support turns out to be impossible. There will always be some build systems that scan includes such as #include <format> in order to build an include graph, regardless of any surrounding #ifdef-type preprocessor macros. If we make this design choice, we will break current or future users.
The way forward is to add a new header, "au/std_format.hh", that users can opt in and include. Only users who can support #include <format> would ever do this, so this would be a robust fix.
We also need to upgrade the single file tool to give an option for std::format support, off by default.
Co-authored-by: Michael Hordijk <hordijk@aurora.tech>
|
For clarity (and my own convenience), here's my TODO list to get this out of draft:
|
We can trust users would only include it if their toolchain supports it.
We tried this earlier (#491), but then reverted it (#495). That was
just a few short weeks ago: what changed?
We learned that our old format string syntax wasn't just hard to
implement; it was actually impossible. This led naturally to a new
idea that may not be as flexible, but actually still supports the most
important use cases. Happily, this new idea is very easy to implement
without using any internal implementation details of the formatting
libraries. It even gives us a little (very little) room for evolving
more options for formatting the unit label, such as setting the fill
character, although we don't currently expect to ever need or want them.
I was also able to verify support for
std::formatby generating asingle file, and uploading it to godbolt. This shows that on a
compiler version and setup that includes
std::format, Au supports itvia the
"au/std_format.hh"header. I also updated the single-filescript to have
--no-std-format(which is the default) and--std-formatoptions. A future PR will add versions with/withoutstd::formatsupport to our automatically generated single-fileoptions.
On the CMake side, we need to skip linting for
std_format.hh. It'stoo hard and too complicated to set up reliable rules for when we should
and shouldn't make it available. Therefore, we always make it
available, but we trust that users will only include it if they know
they can support it. That's the point of the opt-in header strategy.
Note, though, that this required us to bump the minimum CMake version
from 3.24 to 3.27.
Fixes #149.