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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
