[PATCH] D95691: Implement P2173 for attributes on lambdas

Tue Mar 2 15:16:24 PST 2021

rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.

================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:269
+def CXXPre2BCompatPedantic :
+  DiagGroup<"c++98-c++11-c++14-c++17-c++20-compat-pedantic", [CXXPre2BCompat]>;

----------------
aaron.ballman wrote:
> rsmith wrote:
> > rjmccall wrote:
> > > rsmith wrote:
> > > > rjmccall wrote:
> > > > > Quuxplusone wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > rjmccall wrote:
> > > > > > > > Uh, I think we're a couple standard releases past the point at which we should have reconsidered this schema.  I guess the problem is that we can't say `-Wpre-c++23-compat` without jumping the gun.  Is there a problem with `-Wc++20-compat` and then having the earlier warning groups imply the later ones?  That seems to be what we do with `-Wc++98-compat`; did we abandon that approach intentionally?
> > > > > > > @rsmith may have more background here. I was following the pattern already in the file, but I tend to agree that this pattern is not leading us somewhere good. FWIW, I ran into a similar situation with this on the C side of things in D95396, so we should probably be consistent there too.
> > > > > > My understanding is that the //command-line user// is expected to pass
> > > > > > - `clang++ -std=c++20 -Wc++11-compat` to indicate "I want //actually// to compile in C++20 mode, but give me warnings about anything that would prevent compiling in C++11 mode"
> > > > > > - `clang++ -std=c++17 -Wc++14-compat` to indicate "I want //actually// to compile in C++17 mode, but give me warnings about anything that would prevent compiling in C++14 mode"
> > > > > > - `clang++ -std=c++14 -Wc++20-compat` to indicate "I want //actually// to compile in C++14 mode, but give me warnings about anything that would prevent compiling in C++20 mode" — EXCEPT that I think this is not supported. My impression is that forward-compatibility warnings are generally just rolled into `-Wall` and not handled separately beyond that?
> > > > > > 
> > > > > > I don't think any human user is expected to pass `-Wc++98-c++11-c++14-c++17-c++20-compat` by hand; it's just an internal name for a particular subset of `-Wc++98-compat`.
> > > > > > 
> > > > > > IOW, we could choose a new naming scheme for it, but that would be a purely internal change that won't affect how command-line users interact with Clang at all (for better and for worse).
> > > > > Diagnostic groups can both directly contain diagnostics and imply other diagnostic groups, so I don't think there's any reason to make a dedicated group just to contain the new diagnostics in e.g. `-Wc++14-compat` except to allow someone turn on those warnings separately.  And it does show up to users as the warning group under `-fdiagnostics-show-option` (which is the default).
> > > > @Quuxplusone's comment describes the intent. `-std=c++20 -Wc++14-compat` should give a more or less complete list of reasons why the code would not compile in C++14 (at least on the language side; we don't check for stdlib compatibility). The other direction -- `-std=c++11 -Wc++14-compat` -- is more of a best-effort check for things that we've seen cause problems in practice and can easily detect. (As a consequence, I don't think there's any subset/superset relation between `-Wc++X-compat` and `-Wc++Y-compat`.)
> > > > 
> > > > I'd be happy to see these groups renamed to `-Wpre-c++20-compat` or similar. Warning group synonyms are relatively cheap, so I wouldn't be worried about adding a `-Wpre-c++2b-compat` now and renaming it to `-Wpre-c++23-compat` flag later.
> > > > 
> > > > (As an aside, it'd be handy if there were some way to mark a `DiagGroup` as existing only for grouping purposes, so that we could avoid exposing a `-W` flag for cases where groups are added for internal reasons.)
> > > Okay.  It looks like `-Wc++X-compat` is consistently (1) all the this-feature-used-not-to-exist diagnostics from C++X and later plus (2) warnings about deprecation and semantic changes introduced by exactly version X.  This seems like an unfortunate pairing, basically caused by the option names not being very clear about what kind of compatibility they mean.  If we want @Quuxplusone's interpretation, which I agree is a natural human interpretation of those command lines, we'll need special support for it in diagnostic-option handling, so that we include specific diagnostics based on the relationship between the option and the language version.
> > > 
> > > There is a natural subset relationship between the this-feature-used-not-to-exist groups; we're just not taking advantage of it at all.
> > (2) sounds like a bug. Maybe we should add `CXXPostXYCompat` groups, symmetric to the `CXXPreXYCompat` groups, to better handle that?
> > 
> > I'm not sure about the need for special support in diagnostic option handling -- we don't ever produce a "you're using a feature that wasn't in an older standard mode" warning unless we're in the newer mode, and we don't ever produce a "you're using a feature that will change / go away in a newer standard mode" warning unless we're in the older mode.
> > 
> > I think it'd be reasonable to take advantage of the subset relationships. Back when there were only a couple of C++ language standards we cared about, the difference between linear and quadratic growth didn't really matter, but we're past that point now.
> In terms of what's reasonable for this patch, what's our path forward? It sounds like we'd like to see `CXXPre2bCompat` that's spelled `-Wpre-c++2b-compat` (and same for pedantic), and then we'll add aliases for the other language standard modes in a follow-up?
I'm happy with what you have here, so long as the cleanup is actually done. I don't think this inconsistent state is OK for the longer term.

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

https://reviews.llvm.org/D95691