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

Fri Sep 9 08:56:32 PDT 2022

ldionne added a comment.

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. 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. 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.

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`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133425