[PATCH] D133425: Silence -Wctad-maybe-unsupported stemming from system headers

Fri Sep 9 09:46:37 PDT 2022

dblaikie added a comment.

In D133425#3780435 <https://reviews.llvm.org/D133425#3780435>, @ldionne wrote:

> In D133425#3779121 <https://reviews.llvm.org/D133425#3779121>, @dblaikie wrote:
>
>>> One thing I don't understand in the current state of things is why the diagnostic fires at all inside system headers. I thought warnings in system headers were discarded?
>>
>> It doesn't fire if the location of the diagnostic (the place that's using CTAD) is in a system header. But it does fire if that place is outside a system header, but the template that is being used is in a system header (see the SysHeaderObj example above - maybe the naming is confusing? It's a `SysHeaderClass` but the object (`sho`) is not in a system header).
>
> Oh, right, so my confusion was about point-of-definition vs point-of-use.
>
>>> FWIW, my "objection" that we should not silence the warning when users try using CTAD with arbitrary types in `std::` remains -- I think it would be making a disservice to users to let them use CTAD with classes that have not been designed with that in mind. At the end of the day, I think I'm advocating for individual classes opting-out of the warning, while the patch as currently formulated forces all classes in system headers to be opted-out of the warning.
>>
>> I think the problem is that not all system headers match the style required for this constraint - so in the interests of compatibility with the complex world of existing system headers, it's suitable not to enforce this stylistic constraint (requiring explicit deduction guides even when the default is what the template author intended to allow CTAD) on system headers. With the knowledge that this does introduce false negatives, but that we don't have a strong enough signal to avoid them.
>
> I think this might come down to a difference in opinion here. Personally, I'd rather restrict the use of CTAD only to the small subset of classes that are meant to work with it, and warn everywhere else. And I'm fine if a system library has to jump through a few hoops in order to avoid warning on a class that does work well with CTAD.

But that's the issue, and generally why warnings are suppressed in system headers - as a user, you can't fix the system headers you depend on. So it usually ends up as a "the user can't use this feature with -Werror/the warning enabled, so to write valid code they turn off or ignore the warning".

> Allowing users to use CTAD on a class is like a contract given to them, and the fact that this contract is implicitly assumed by the language is often cited as a questionable design decision in C++17. I think this warning is useful and if the special markup had been there on the few classes that need it (like `lock_guard`), we wouldn't be having that discussion at all because I don't think anybody actually wants to use CTAD with random types in a system library.
>
> In particular, the warning is useful in general user code, and I don't really follow why the fact that a type is authored in a system library would suddenly make it more okay to use with CTAD.

It's more an argument from false positives in the warning because libraries outside your control may not adhere to this coding convention (which it is - like parentheses around assignment in an if condition, it's a somewhat arbitrarily chosen syntax intended to communicate explicit intent rather than the implicit default - but it's something all the authors need to agree on, not something that would be known independently by the author of a library/happen to be just general good practice when writing code in the absence of this warning depending on that spelling/signal)

> Types in system libraries are not different from user types w.r.t. CTAD -- several types are just not designed with CTAD in mind.
>
> Finally, I'd like to note that this warning only triggers if the user explicitly passes `-Wctad-maybe-unsupported`. It's not enabled by `-Wall` or `-Wextra`. I would argue that users that go out of their way to enable this warning probably share my sentiment that CTAD is sometimes harmful, and they probably want to be warned about questionable uses even for classes defined in a system library.

Maybe? But if they're stuck with system libraries that don't use this convention, this extra coverage of the warning may prevent them from using the warning at all, even for their non-system-header code.

> Anyway, I think I've made my opinion clear and I don't think I have much more to bring to the table. I'll ship D133535 <https://reviews.llvm.org/D133535> regardless, but if this patch ships, I think we should have some way of enabling the warning for types in system headers. For example, folks using `-Wctad-maybe-unsupported` should really be warned about using CTAD on a type like `std::reverse_iterator`.

Yeah, that might be a way forward - splitting the warning in two - have one level that's the current even-in-system-headers behavior, then a subset that's the GCC-behavior. (probably the current flag name should match the GCC behavior, as much as that's a break in compatibility with previous-clang - but I could see the counterargument that we should keep the name/semantics matching previous clang and add a separate GCC-incompatible name for the GCC-compatible behavior... )

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133425