[PATCH] D111400: [Clang][C++2b] P2242R3: Non-literal variables [...] in constexpr

[Aaron Ballman via Phabricator via cfe-commits]

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