[PATCH] D131727: Enable -Wctad-maybe-unsupported in LLVM build

Sat Aug 13 17:00:24 PDT 2022

jloser added a comment.

In D131727#3721438 <https://reviews.llvm.org/D131727#3721438>, @dblaikie wrote:

> In D131727#3720994 <https://reviews.llvm.org/D131727#3720994>, @philnik wrote:
>
>> In D131727#3720886 <https://reviews.llvm.org/D131727#3720886>, @dblaikie wrote:
>>
>>> Updated with the libcxx changes necessary to get a clean build - but I'm still confused by:
>>>
>>> 1. why the clang warning fired on these at all (I haven't looked into that in detail, but I can probably figure it out with some closer inspection)
>>> 2. why libcxx doesn't seem to apply `LLVM_ENABLE_WERROR` & what that means with regards to libcxx's policy on warning cleanliness, etc..
>>
>>
>>
>> 1. The warnings probably fire because we don't consider the headers to be system headers when building the library.
>
> Oooh yeah, good call - I hadn't thought about that part of it. I think when I saw the description in the Google style guide that said "all types in the std namespace are assumed to have opted in" I took that literally, but you're right that it's probably a system header filter rather than a std namespace filter.
>
> Would libc++ want to compile itself as system headers? Or does it generally want all/most of the extra warnings when compiling its own implementation, I guess?
>
>> 2. We have `LIBCXX_ENABLE_WERROR` for that. I don't know why we ignore `LLVM_ENABLE_WERROR`, but it probably has something to do with GCC not building libc++ cleanly.
>
> Ah, weird - yeah, would be nice to unify those at some point - other projects in LLVM have the same issues with GCC warnings and we disable them when necessary/appropriate rather than having `LLVM_ENABLE_WERROR` ignored when the compiler isn't clang or anything like that.
>
>> For example we get warnings that `__attribute__((init_priority(100)))` is implementation-reserved. When using clang we build with `-Werror` in the CI.
>>
>> I think libc++ shouldn't be built with `-Wctad-maybe-unsupported`, since we can generally assume that using CTAD is OK.
>
> Yeah, that was my feeling too but I wasn't sure where/how to disable it in the libc++ build. But I guess maybe I can add it to this list ( https://github.com/llvm/llvm-project/blob/eaf0aa1f1fbd06fbd66417f2c9d50d3074969824/libcxx/CMakeLists.txt#L592  (updated this review to include that change)) and I don't have to worry about compiler compatibility checks because libcxx uses the just-built clang, so it's not going to run against an outdated clang that'll complain that it doesn't know the warning flag?
>
>> BTW it would help if you ping the #libc <https://reviews.llvm.org/tag/libc/> group next time you want to ask something. That way all the libc++ developers get an email.
>
> Oh, right - thanks for the pointer!

`libc++` doesn't always use the just-built clang. We keep support for the last two versions of clang, so the compiler compatibility checks would be needed, right? Was the flag added in llvm14 or llvm15?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131727/new/

https://reviews.llvm.org/D131727