-
Notifications
You must be signed in to change notification settings - Fork 191
Add {}-style string formatting for logging macros #3125
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
base: dev
Are you sure you want to change the base?
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.
clang-tidy made some suggestions
f3e714c to
34f5c0c
Compare
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.
clang-tidy made some suggestions
Previously, formatting log messages with dynamic values required manual string concatenation or the use of `std::ostringstream`, which can be verbose and less readable.
34f5c0c to
d52c0aa
Compare
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.
clang-tidy made some suggestions
|
Something like this would be nice. Having recently started playing around with However, the implementation you're suggesting seems quite primitive, if I'm not mistaken? I was assuming we'd instead opt for something with a few more features, such as the fmt library - is that an option? The license seems pretty permissive, and it seems to be very widely used. |
Yes, this implementation is indeed quite simplistic. Integrating proper formatting support, as mentioned in #2951, using the |
|
I'd be concerned about there being a distinction in syntax between construction of a string within one of these macros versus construction of a If there's a comparable capability in C++20, and compilation from source is increasingly restricted to developers only, then I'd have thought that changing the language standard would make more sense? Though we probably have a lot more to resolve with 17 before progressing further. |
|
Personally, I would be in favour of upgrading the standard of compilation to C++20 (which doesn't necessarily mean utilising all of its features). Compiler support is quite good nowadays(modulo C++20 modules). |
|
That's a good suggestion, I've just been getting up to speed with the C++20 standard, and there are a few nice features in there that would be good to have access to. Compilation is still tricky on older versions of the OS, but these are diminishing concerns, especially as most users will install pre-compiled binaries, and we can provide feature binaries on demand. There is now first-class support for C++20 modules in CMake, which I think would be good to explore in due course (not for now though, that will require quite a bit of rejigging). Either way, we should be able to compile against C++20 now with minimal changes, and we can gradually start to make use of the new functionality as opportunities arise. Happy to go with that plan! |
|
I checked the minimum compiler versions needed for |
|
If
|
|
Just jotting down some thoughts about this:
Basically, I'm happy with either using the fmt library or moving to C++20 and using |
d52c0aa to
50587ca
Compare
70031c3 to
6bf4cec
Compare
50587ca to
d52c0aa
Compare
This PR introduces a new string formatting utility,
MR::format_string, and integrates it into the primary logging macros (CONSOLE,FAIL,WARN,INFO,DEBUG) to provide a more modern and convenient way to format log messages.Previously, formatting log messages with dynamic values required manual string concatenation or the use of
std::ostringstream, which can be verbose and less readable.Now, instead of this:
you can do the following:
Perhaps this could be made more robust to check at compile time whether the number of
{}parameters match the number of arguments provided (amongst other things), but I think that will make the implementation significantly more complex. This is a good enough solution until we move onto C++20, which introducesstd::format(an alternative would be to implement #2951).