fmtlib / fmt

A modern formatting library
https://fmt.dev
Other
20.43k stars 2.45k forks source link

Clang 18 rejects `operator"" _a` #3607

Closed danakj closed 12 months ago

danakj commented 1 year ago
third_party/fmt/include/fmt/format.h:4417:27: warning: identifier '_a' preceded by whitespace in a literal operator declaration is deprecated [-Wdeprecated-literal-operator]
 4417 | constexpr auto operator"" _a(const char* s, size_t) -> detail::udl_arg<char> {
      |                ~~~~~~~~~~~^~
      |                operator""_a
1 warning generated.

The fix is to change operator"" _a to operator""_a

vitaut commented 1 year ago

Could you provide a godbolt repro?

danakj commented 1 year ago

It's trivial to make a repro of the warning outside of fmtlib but I am having trouble getting format.h to create that UDL I guess and I am not sure why.

https://godbolt.org/z/WbaoTT9KP

Here's a CI run where I found the warning trip: https://github.com/chromium/subspace/actions/runs/5947977722/job/16130912813?pr=330

It will only occur when FMT_USE_NONTYPE_TEMPLATE_ARGS is false. The UDL when true has no whitespace, which is correct syntax. https://github.com/fmtlib/fmt/blob/0bffed89579ac8a210379fd640569e4b75fe28c8/include/fmt/format.h#L4425-L4434

You would think FMT_USE_NONTYPE_TEMPLATE_ARGS would be true with clang 18: https://github.com/fmtlib/fmt/blob/0bffed89579ac8a210379fd640569e4b75fe28c8/include/fmt/core.h#L234-L243

I am compiling with C++20 enabled, I am not sure what is different on Github CI vs Godbolt. Any ideas?

vitaut commented 1 year ago

godbolt might be hiding warnings from the library but your repro is good enough. Unfortunately per discussion on the PR gcc requires a space there so I'm not sure we can do anything about it.

vitaut commented 1 year ago

I guess one options would be to only enable UDLs on gcc 4.9 or higher in https://github.com/fmtlib/fmt/blob/0bffed89579ac8a210379fd640569e4b75fe28c8/include/fmt/format.h#L155

danakj commented 1 year ago

I am building clang trunk on my mac to see if I can reproduce there, and if so, explain why or if FMT_USE_NONTYPE_TEMPLATE_ARGS is indeed false.

danakj commented 1 year ago

No luck, clang ToT on Mac is not hitting the warning.

danakj commented 1 year ago

I did some testing in CI: https://github.com/chromium/subspace/actions/runs/5957000756/job/16158898370?pr=330

#if defined(__cpp_nontype_template_args)
#warning __cpp_nontype_template_args
#endif
#if FMT_GCC_VERSION >= 903
#warning FMT_GCC_VERSION >= 903
#endif
#if FMT_CPLUSPLUS >= 201709L
#warning FMT_CPLUSPLUS >= 201709L
#endif
#if __cpp_nontype_template_args >= 201911L
#warning __cpp_nontype_template_args >= 201911L
#endif
#if defined(__NVCOMPILER)
#warning __NVCOMPILER
#endif
#if defined(__LCC__)
#warning __LCC__
#endif

Prints

/home/runner/work/subspace/subspace/sus/lib/lib.cc:25:2: error: __cpp_nontype_template_args [-Werror,-W#warnings]
   25 | #warning __cpp_nontype_template_args
      |  ^
/home/runner/work/subspace/subspace/sus/lib/lib.cc:31:2: error: FMT_CPLUSPLUS >= 201709L [-Werror,-W#warnings]
   31 | #warning FMT_CPLUSPLUS >= 201709L
      |  ^
2 errors generated.
[15/185 1.0/sec] Building CXX object sus/CMakeFiles/subspace.dir/lib/lib.cc.o
/home/runner/work/subspace/subspace/sus/lib/lib.cc:25:2: warning: __cpp_nontype_template_args [-W#warnings]
   25 | #warning __cpp_nontype_template_args
      |  ^
/home/runner/work/subspace/subspace/sus/lib/lib.cc:31:2: warning: FMT_CPLUSPLUS >= 201709L [-W#warnings]
   31 | #warning FMT_CPLUSPLUS >= 201709L
      |  ^
2 warnings generated.

Meaning __cpp_nontype_template_args is defined but is not >= 201911L which is why it's going into the other path there.

danakj commented 1 year ago

I guess because it is not done. https://github.com/llvm/llvm-project/issues/54297#issuecomment-1578920133

So they started warning on the operator"" syntax but the branch choices made in fmtlib then clash with gcc. Even though I think it is implemented enough for fmtlib's use anyway. I wonder if maybe hardcoding clang >= 16 or something to go the path without the warning?

danakj commented 1 year ago

I got wondering if __cpp_nontype_template_args >= 201911L is false on my CI what is happening on my mac. It is also false there, and the operator"" _a version is being used. For some reason -Wdeprecated-literal-operator is enabled in the clang binaries I pull from llvm, but in the clang I built myself, it's not enabled by default. Passing it on the command line also hits the warrning:


$HOME/s/llvm/install/bin/clang++ -DFMT_SHARED -I$HOME/s/subspace -I$HOME/s/subspace/third_party/googletest -I$HOME/s/subspace/third_party/fmt/include -Is/subspace/third_party/nanobench/src/include -isystem $HOME/s/subspace/third_party/googletest/googletest/include -isystem $HOME/s/subspace/third_party/googletest/googletest -isystem /Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/include/c++/v1 -isystem s/llvm/install-18/lib/clang/18/include -isystem /Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/include -g -std=gnu++20 -arch arm64 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk -mmacosx-version-min=13.4 -fno-rtti -Wno-nullability-completeness -Werror -MD -MT bench/CMakeFiles/bench.dir/__/sus/lib/lib.cc.o -MF bench/CMakeFiles/bench.dir/__/sus/lib/lib.cc.o.d -o bench/CMakeFiles/bench.dir/__/sus/lib/lib.cc.o -c $HOME/s/subspace/sus/lib/lib.cc -Wdeprecated-literal-operator
In file included from $HOME/s/subspace/sus/lib/lib.cc:21:
In file included from $HOME/s/subspace/sus/num/signed_integer.h:25:
$HOME/s/subspace/third_party/fmt/include/fmt/format.h:4417:27: error: identifier '_a' preceded by whitespace in a literal operator declaration is deprecated [-Werror,-Wdeprecated-literal-operator]
 4417 | constexpr auto operator"" _a(const char* s, size_t) -> detail::udl_arg<char> {
      |                ~~~~~~~~~~~^~
      |                operator""_a
danakj commented 1 year ago

In LLVM:

commit 0d9919d362a7a70b2a7970861d897ecc47ec9e4d
Author: Reid Kleckner <rnk@google.com>
Date:   Tue Aug 22 18:10:41 2023 -0700

    Revert "[Clang] CWG1473: do not err on the lack of space after operator"""

    This reverts commit f2583f3acf596cc545c8c0e3cb28e712f4ebf21b.

    There is a large body of non-conforming C-like code using format strings
    like this:

      #define PRIuS "zu"
      void h(size_t foo, size_t bar) {
        printf("foo is %"PRIuS", bar is %"PRIuS, foo, bar);
      }

    Rejecting this code would be very disruptive. We could decide to do
    that, but it's sufficiently disruptive that I think it requires
    gathering more community consensus with an RFC, and Aaron indicated [1]
    it's OK to revert for now so continuous testing systems can see past
    this issue while we decide what to do.

    [1] https://reviews.llvm.org/D153156#4607717
commit f2583f3acf596cc545c8c0e3cb28e712f4ebf21b
Author: Po-yao Chang <poyaoc97@gmail.com>
Date:   Thu Aug 17 22:57:42 2023 +0800

    [Clang] CWG1473: do not err on the lack of space after operator""

    In addition:
      1. Fix tests for CWG2521 deprecation warning.
      2. Enable -Wdeprecated-literal-operator by default.

    Differential Revision: https://reviews.llvm.org/D153156
commit 5ce5e983f82c802e44faa8ed42d605d70c045ba9
Author: Po-yao Chang <poyaoc97@gmail.com>
Date:   Thu Jul 13 23:42:51 2023 +0800

    [Clang] Add warnings for CWG2521

    1. Teach -Wuser-defined-literals to warn on using double underscores in
       literal suffix identifiers.
    2. Add -Wdeprecated-literal-operator to warn about the use of the first
       grammar production of literal-operator-id, which defaults to off for now.

    Differential Revision: https://reviews.llvm.org/D152632

After that revert by @rnk my CI is green with clang 18, I guess I had unlucky timing there. Not sure if you want to make fmtlib confirming by removing the whitespace - it seems like a good idea to do still to me but I can understand otherwise.