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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added subscribers: EricWF, rsmith.
dblaikie added a comment.

In D133425#3815118 <https://reviews.llvm.org/D133425#3815118>, @aaron.ballman wrote:

> In D133425#3780579 <https://reviews.llvm.org/D133425#3780579>, @dblaikie wrote:
>
>> 
>
> 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... )
>
> I've been thinking on this a bit, and I think this is the best way forward. I don't have a strong opinion on whether we retain the behavior we had here or not. This diagnostic is currently ignored by default, so it's unlikely to have significant use in the wild. Any preference from the peanut gallery?

Google's managed to use the warning with its current behavior for however many years it's been implemented, so we'd probably gain something by continuing to use the more aggressive version (I'm sort of surprised we've been able to use it as long as we have - basically the moment LLVM switched to C++17, the LLVM codebase had violations of the warning, so I'm not sure why we haven't seen that come up in lots of other third party code (which we flag as system headers to reduce the constraints on it generally) - perhaps just less aggressive adoption of newer language feature usage in the other third party code we depend on). But I'm not sure we'd miss it greatly either? Hard to say. The cost is a long term one in terms of maintenance of libraries - code depending on the unintended surface area then, one day, that surface area wanting to change but being stuck (well, higher cost for the change/migration) due to all the accumulated usage.

I was going to say we could keep the full functionality under the current flag name, adding a subflag for the control over the "skipping system headers" part - but that would be a bit inconsistent (I guess usually sub flags add more cases that warn, rather than removing them) and we'd still have different behavior than GCC for the same flag name, which would be unfortunate. So the right thing is probably to change the behavior of this flag, and (potentially) have a subflag with the more aggressive/old behavior.

@EricWF @rsmith as the original author/reviewer of the feature and vested parties on the Google side.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133425