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

Clang 18 rejects operator"" _a #3607

Closed
danakj opened this issue Aug 23, 2023 · 10 comments · Fixed by #3610
Closed

Clang 18 rejects operator"" _a #3607

danakj opened this issue Aug 23, 2023 · 10 comments · Fixed by #3610

Comments

@danakj
Copy link
Contributor

danakj commented Aug 23, 2023

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

danakj added a commit to danakj/fmt that referenced this issue Aug 23, 2023
Clang 18 warns about this whitespace.

Resolves fmtlib#3607
danakj added a commit to danakj/fmt that referenced this issue Aug 23, 2023
Clang 18 warns about this whitespace.

Resolves fmtlib#3607
@vitaut
Copy link
Contributor

vitaut commented Aug 23, 2023

Could you provide a godbolt repro?

@danakj
Copy link
Contributor Author

danakj commented Aug 23, 2023

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.

fmt/include/fmt/format.h

Lines 4425 to 4434 in 0bffed8

# if FMT_USE_NONTYPE_TEMPLATE_ARGS
template <detail_exported::fixed_string Str> constexpr auto operator""_a() {
using char_t = remove_cvref_t<decltype(Str.data[0])>;
return detail::udl_arg<char_t, sizeof(Str.data) / sizeof(char_t), Str>();
}
# else
constexpr auto operator"" _a(const char* s, size_t) -> detail::udl_arg<char> {
return {s};
}
# endif

You would think FMT_USE_NONTYPE_TEMPLATE_ARGS would be true with clang 18:

fmt/include/fmt/core.h

Lines 234 to 243 in 0bffed8

#ifndef FMT_USE_NONTYPE_TEMPLATE_ARGS
# if defined(__cpp_nontype_template_args) && \
((FMT_GCC_VERSION >= 903 && FMT_CPLUSPLUS >= 201709L) || \
__cpp_nontype_template_args >= 201911L) && \
!defined(__NVCOMPILER) && !defined(__LCC__)
# define FMT_USE_NONTYPE_TEMPLATE_ARGS 1
# else
# define FMT_USE_NONTYPE_TEMPLATE_ARGS 0
# endif
#endif

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

@vitaut
Copy link
Contributor

vitaut commented Aug 23, 2023

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
Copy link
Contributor

vitaut commented Aug 23, 2023

I guess one options would be to only enable UDLs on gcc 4.9 or higher in

# if (FMT_HAS_FEATURE(cxx_user_literals) || FMT_GCC_VERSION >= 407 || \

@danakj
Copy link
Contributor Author

danakj commented Aug 23, 2023

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
Copy link
Contributor Author

danakj commented Aug 23, 2023

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

@danakj
Copy link
Contributor Author

danakj commented Aug 23, 2023

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
Copy link
Contributor Author

danakj commented Aug 23, 2023

I guess because it is not done. llvm/llvm-project#54297 (comment)

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
Copy link
Contributor Author

danakj commented Aug 24, 2023

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
Copy link
Contributor Author

danakj commented Aug 24, 2023

In LLVM:

commit 0d9919d362a7a70b2a7970861d897ecc47ec9e4d
Author: Reid Kleckner <[email protected]>
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 <[email protected]>
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 <[email protected]>
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.

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

Successfully merging a pull request may close this issue.

2 participants