aaron.ballman accepted this revision. aaron.ballman added a comment. I don't spot anything overly concerning in this patch, I believe it LGTM as well.
================ Comment at: clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3-2b.cpp:38 + if (!b) + NonLiteral n; +} ---------------- hubert.reinterpretcast wrote: > cor3ntin wrote: > > hubert.reinterpretcast wrote: > > > For consistency, this should warn (under `-Wpre-c++2b-compat`). > > I though we decided *not* to do that > Actually, I think we only decided that it should always be an error in older > modes (and we didn't decide not to add a compat warning). That is, the > extension and compat warnings just happen to be paired currently in the > patch. Now that the code has cleared out of my system a bit, I believe that > there is no fundamental reason for the two warnings to be paired. > > I'm fine with getting this patch landed and then fixing this after. Maybe > @aaron.ballman would comment as well. I would also be fine with addressing that after this patch lands. ================ Comment at: clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp:1-3 +// RUN: %clang_cc1 -verify -fcxx-exceptions -triple=x86_64-linux-gnu -std=c++11 -Werror=c++14-extensions -Werror=c++20-extensions -Werror=c++2b-extensions %s +// RUN: %clang_cc1 -verify -fcxx-exceptions -triple=x86_64-linux-gnu -std=c++14 -DCXX14 -Werror=c++20-extensions -Werror=c++2b-extensions %s +// RUN: %clang_cc1 -verify -fcxx-exceptions -triple=x86_64-linux-gnu -std=c++20 -DCXX14 -DCXX20 -Werror=c++2b-extensions %s ---------------- hubert.reinterpretcast wrote: > cor3ntin wrote: > > hubert.reinterpretcast wrote: > > > cor3ntin wrote: > > > > hubert.reinterpretcast wrote: > > > > > hubert.reinterpretcast wrote: > > > > > > cor3ntin wrote: > > > > > > > hubert.reinterpretcast wrote: > > > > > > > > The use `-Werror` hides potential issues where the error is > > > > > > > > categorically produced (instead of under the warning group). > > > > > > > > > > > > > > > > Considering that there is a relevant design consistency issue > > > > > > > > of exactly that sort right now that this test could have > > > > > > > > highlighted (but didn't), a change to stop using `-Werror` may > > > > > > > > be prudent. > > > > > > > This seems inconsistent with how that file is currently > > > > > > > structured though > > > > > > I meant to change the entire file to check for warnings instead of > > > > > > errors. I guess that really should be a separate patch. > > > > > I guess the change will cause the "never produces a constant > > > > > expression" warning unless if that is suppressed with > > > > > `-Wno-invalid-constexpr`. > > > > Yes, we could cleanup this entire file to get rid of the #ifdef, then > > > > change how warnings are diagnosed. > > > I am not in favour of getting rid of the `#ifdef`s. They still tell us > > > that the "conformance" warnings are associated with the right modes. > > To be clear, i meant using `//cxx20-warning` and things like that instead, > > which is functionally equivalent. Does that make sense? > Yes it does. I think that would be good (but does not need to be part of this > patch). FWIW, I also agree that it'd be good to use `-verify=something` instead of `#ifdef` (but not strictly necessary for this patch). ================ Comment at: clang/test/SemaCXX/constant-expression-cxx2b.cpp:118 + +} // namespace eval_goto + ---------------- hubert.reinterpretcast wrote: > cor3ntin wrote: > > hubert.reinterpretcast wrote: > > > Move `#endif` to here (from below) so the explicitly-`constexpr` lambda > > > cases are also tried in C++20 mode. > > I mean sure I can do that, but what are we testing here? > We're testing that the extension behaviour is actually extended to > explicitly-`constexpr` lambdas in C++20 mode despite the non-application (in > C++20 mode) of the new rules when determining whether a lambda is implicitly > `constexpr`. > > Having the test reinforces that the behaviour is as intended, which serves a > design documentation purpose. > > The test also arises when applying closed-box testing methodology to > speculate how a desired behaviour may have been implemented in a way that > also leads to undesired behaviour. In other words, maybe the code keyed off > too much on being in a lambda body. > > Yes, I know we can read the code, but then again that's today's code and not > necessarily tomorrow's. > Having the test reinforces that the behaviour is as intended, which serves a > design documentation purpose. We do this in our tests somewhat regularly, but please be sure to add comments to explain that the test is serving a design documentation purpose, otherwise it can be easy to later go "oh, this is obviously broken nonsense so it's fine that behavior changed" by accident. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111400/new/ https://reviews.llvm.org/D111400 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
