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

Compile checks in c++20 are kind of lacking #2640

Closed
YarikTH opened this issue Dec 9, 2021 · 13 comments
Closed

Compile checks in c++20 are kind of lacking #2640

YarikTH opened this issue Dec 9, 2021 · 13 comments

Comments

@YarikTH
Copy link

YarikTH commented Dec 9, 2021

I massively rewrote ostream based logging in my codebase using _format literal. And suddenly I found out that this is not checked at all. I really like the idea of compile-time checked format because it allows seeing all the problems immediately at the compilation stage, instead of waiting for a crash somewhen in future when the code with {fmt} call will be executed. And it's sad that currently it's almost doesn't working.

Here is a list of compile-time checks that works:

https://godbolt.org/z/cc8MPrGc9

#include <fmt/format.h>

using namespace fmt::literals;

void func() {
    fmt::format( "{}", 1, 2 );                      // (1) UNCHECKED
    fmt::format( "{} {} {}", 1, 2 );                // (2) checked
    fmt::format( "{:d} {}", "", 2 );                // (3) checked
    fmt::format( "{addr2}", fmt::arg("addr", 1) );  // (4) UNCHECKED
    fmt::format( "{addr2}", "addr"_a = 1 );         // (5) UNCHECKED

    "{}"_format( 1, 2 );                            // (6) UNCHECKED
    "{} {} {}"_format( 1, 2 );                      // (7) UNCHECKED
    "{:d} {}"_format( "", 2 );                      // (8) UNCHECKED
    "{addr2}"_format( fmt::arg("addr", 1) );        // (9) UNCHECKED
    "{addr2}"_format( "addr"_a = 1 );               // (10) UNCHECKED
}
  • 1,6 - too many arguments. Unchecked at all
  • 2,7 - too few arguments. Checked only in fmt::format
  • 3,8 - incorrect format flags. Checked only in fmt::format
  • 4,9 - incorrectly named arguments using fmt::arg. Unchecked at all
  • 5,10 - incorrectly named arguments using _a literal. Unchecked at all

Is there a chance that some unchecked scenarios, especially with _format literals would become checked? Now it seems quite inconsistent and confusing.

P.S. I checked only gcc10 and gcc11 with trunk {fmt}. So maybe they don't work only under some list of compilers.

@vitaut
Copy link
Contributor

vitaut commented Dec 9, 2021

All of these are expected. It's OK to have unused arguments, named arguments cannot be checked due to C++ limitations and UDL-based API is not recommended at all and will likely be deprecated.

@vitaut vitaut closed this as completed Dec 9, 2021
@alexezeder
Copy link
Contributor

alexezeder commented Dec 9, 2021

All of these are expected

@vitaut, I'm afraid I have to disagree with you here since only some of them are expected:

1,6 - too many arguments. Unchecked at all - definitely expected, but some strict mode can be helpful, because checking it on the client-side requires excess string parsing pass (for counting)
2,7 - too few arguments. Checked only in fmt::format - not expected that _format() doesn't have these checks
3,8 - incorrect format flags. Checked only in fmt::format - not expected that _format() doesn't have these checks
4,9 - incorrectly named arguments using fmt::arg. Unchecked at all - expected because of C++ limitations
5,10 - incorrectly named arguments using _a literal. Unchecked at all - not expected for both fmt::format() and _format()

named arguments cannot be checked due to C++ limitations

They cannot be checked for fmt::arg only, since we cannot get the name at compile-time. But with _a, we can get it at compile-time the same way as FMT_COMPILE does.

UDL-based API is not recommended at all and will likely be deprecated

I don't remember that discouragement of UDL-based usages is stated somewhere. 😏
Maybe literals were added only to implement string checking at compile-time in past, therefore they may not seem very useful right now. But, besides crucial _a features at compile-time, they also provide another (for some people even better) interface. So it wouldn't be nice if they become deprecated.
Anyway, since we still have _format right now, it probably should implement the same checks as fmt::format.

@vitaut
Copy link
Contributor

vitaut commented Dec 9, 2021

_a is OK and we could extend checks there (a PR would be welcome) but _format mostly exists for compatibility and should be deprecated. I don't think it's worth doing anything there.

@vitaut
Copy link
Contributor

vitaut commented Dec 9, 2021

Clarified that _format is deprecated in a9c7b9b.

@vitaut
Copy link
Contributor

vitaut commented Dec 9, 2021

Added a note regarding named arguments to #2390 which already tracks expanding compile-time checks.

@alexezeder
Copy link
Contributor

Clarified that _format is deprecated in a9c7b9b.

Shouldn't FMT_DEPRECATED also be used there?

@YarikTH
Copy link
Author

YarikTH commented Dec 10, 2021

Or [[not_recommended("will likely be deprecated")]]? Sorry, could not resist)

@vitaut
Copy link
Contributor

vitaut commented Dec 10, 2021

Shouldn't FMT_DEPRECATED also be used there?

Sure, a PR is welcome =).

@NikolausDemmel
Copy link

but _format mostly exists for compatibility and should be deprecated. I don't think it's worth doing anything there.

Just curious what was the rationale of deprecating the alternative string literal API? So far I almost exclusively used this API and was surprised to now get deprecation warnings. To me it feels very natural in particular if you are used to "...".format(...) in Python.

@vitaut
Copy link
Contributor

vitaut commented Feb 23, 2022

Mostly because it's inconsistent with all the other formatting functions (and std::format) and hasn't been well-supported as the current issue clearly shows. Note that you can easily implement a UDL wrapper around a formatting function yourself:

auto operator"" _format(const char* s, size_t n) {
  return [=](auto&&... args) {
    return fmt::format(fmt::runtime(std::string_view(s, n)), args...);
  };
}

https://godbolt.org/z/KndTKPe43

This way you'll be able to easily migrate to std::format in the future.

@NikolausDemmel
Copy link

I see. That's a good point you make. I like the option to do a custom wrapper, depending on project needs, which also considers migration to std::format, as you say. Thanks for even providing an implementation :-).

Thanks again to you and all contributors for libfmt, it's much appreciated!

@NikolausDemmel
Copy link

I noticed fmt::runtime is only available in recent libfmt versions and in one of my projects I need compatibility with older libfmt (Ubuntu bionic and focal, which come with libfmt version 4.0.0 and 6.1.2, respectively). I've come up with the following solution, which seems to work for those versions. The version ranges I inferred from the documentation. Maybe it's useful for other people that come across this discussion. Any comments are of course also welcome.

inline auto operator"" _format(const char* s, size_t n) {
  return [=](auto&&... args) {
#if FMT_VERSION < 50000
    return fmt::format(s, args...);
#elif FMT_VERSION < 80100
    return fmt::format(std::string_view(s, n), args...);
#else
    return fmt::format(fmt::runtime(std::string_view(s, n)), args...);
#endif
  };
}

@vitaut
Copy link
Contributor

vitaut commented Feb 23, 2022

You could also use vformat to avoid conditional compilation (at least for fmt >= 5.0)

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

No branches or pull requests

4 participants