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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a subscriber: erichkeane.
cor3ntin added a comment.

In D111400#3397533 <https://reviews.llvm.org/D111400#3397533>, @hubert.reinterpretcast wrote:

> LGTM with minor nit. Thank you.

Thanks a lot for the review.
I will probably merge tomorrow unless @aaron.ballman or @erichkeane have further comments



================
Comment at: clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3-2b.cpp:17-22
+constexpr int g() { // expected-error {{constexpr function never produces a constant expression}}
+  goto test;        // expected-note {{subexpression not valid in a constant expression}} \
+           // expected-warning {{use of this statement in a constexpr function is incompatible with C++ standards before C++2b}}
+test:
+  return 0;
+}
----------------
hubert.reinterpretcast wrote:
> cor3ntin wrote:
> > hubert.reinterpretcast wrote:
> > > Still trying to make the choice of tests consistent between the different cases (static local variable, goto statement, etc.) and consistent within files...
> > > 
> > > The test that is here (function will unconditionally evaluate) seems to belong in `constant-expression-cxx2b.cpp`... or at least that is where the equivalent tests for the static local variable case, etc. is (and correctly, I think).
> > > 
> > > The test that should be here is the "conditionally will evaluate" case where the function is not called. That is, this file has tests that check that the warnings are produced purely from the definition being present.
> > I'm not sure I understand. No call is made in that file.
> The case here is unconditional and not called. So, it should be in `constant-expression-cxx2b.cpp` because that is the file with other unconditional (and not called) cases. That a call never produces a constant expression is a property of the evaluation rules.
> 
> The other file currently has `eval_goto::f`, which does have a condition gating the evaluation of the `goto` (and is called). A copy of that function (but no call) would be what fits in this file.
The idea is that `p3-2b.cpp` would check for things without evaluating them, whether `constant-expression-cxx2b.cpp` does evaluate them - aka define vs use. Let me know if that clarifies or if you would still prefer i remove redundant test from `p3-2b.cpp` 


================
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:
> > > 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?


================
Comment at: clang/test/SemaCXX/constant-expression-cxx2b.cpp:118
+
+} // namespace eval_goto
+
----------------
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?


================
Comment at: clang/test/SemaCXX/constant-expression-cxx2b.cpp:208
+
+  constexpr auto non_literal_ok = non_literal(false); // expected-error {{constexpr variable 'non_literal_ok' must be initialized by a constant expression}} \
+                                                      // cxx2a-note {{non-constexpr function}} \
----------------
hubert.reinterpretcast wrote:
> Isn't this the "not 'ok'" case? The naming as `non_literal_ok` is confusing.
You are right, I switched it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111400