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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline 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:
> 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.


================
Comment at: clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3-2b.cpp:16
+}
+
+constexpr int f(int x) {
----------------
hubert.reinterpretcast wrote:
> Add a `NonLiteral` case and a case with a labelled statement and no `goto`?
These tests exist in that same file


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


================
Comment at: clang/test/SemaCXX/constant-expression-cxx2b.cpp:96-123
+int test_in_lambdas() {
+  auto a = [] constexpr {    // expected-error{{constexpr function never produces a constant expression}}
+    static const int m = 32; // expected-note {{control flows through the declaration of a static variable}} \
+                                 // expected-warning {{incompatible with C++ standards before C++2b}}
+    return m;
+  };
+
----------------
hubert.reinterpretcast wrote:
> I think it would be more meaningful to have the explicitly `constexpr` lambda tests in the file (see comment on the later code below) that also runs under C++20 mode. The tests should be structured to demonstrate that the explicitly `constexpr` lambdas are usable in constant expression evaluation even in older modes.
That would just test the extension warnings which we already tests a bunch of time. I'm willing to add more tests but I question whether it would had any value


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111400