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

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:1904-1906
+        if (!SemaRef.LangOpts.CPlusPlus2b &&
+            CheckLiteralType(SemaRef, Kind, VD->getLocation(), VD->getType(),
                              diag::err_constexpr_local_var_non_literal_type,
----------------
This seems to unconditionally error in pre-C++2b modes. For consistency, this should be a `-Wc++2b-extensions` warning.


================
Comment at: clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/dtor.cpp:1
-// RUN: %clang_cc1 -std=c++2a -verify %s
 
----------------
aaron.ballman wrote:
> I think we want to keep testing the C++20 behavior and want new tests for the C++2b behavior. We still need to be sure we're correct in C++20 mode given the differences (even in extensions because `-pedantic-errors` is a thing). So I'd recommend splitting this test into two or using an additional RUN line.
@cor3ntin, I don't think Aaron's comment has been addressed. I think keeping `-std=c++2a` and adding `-Wc++2b-extensions` would be appropriate. I know there are tests elsewhere that also focus on paragraph 3, but this test has coverage specifically for destructors.


================
Comment at: clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3-2b.cpp:38
+  if (!b)
+    NonLiteral n;
+}
----------------
For consistency, this should warn (under `-Wpre-c++2b-compat`).


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


================
Comment at: clang/test/SemaCXX/constant-expression-cxx14.cpp:47
 static_assert(g(2) == 42, "");
-constexpr int h(int n) {
-  static const int m = n; // expected-error {{static variable not permitted in a constexpr function}}
----------------
aaron.ballman wrote:
> I think we should keep this test coverage by adding `-Werror=c++2b-extensions` to the RUN line for C++2b.
@aaron.ballman, the tests have been restored; however, they test the extension behaviour now. The `-Werror=c++2b-extensions` version of the tests are covered by the tests elsewhere that focus on [dcl.constexpr] paragraph 3.


================
Comment at: clang/test/SemaCXX/constant-expression-cxx2b.cpp:3-13
+constexpr int f(int n) {  // expected-error {{constexpr function never produces a constant expression}}
+  static const int m = n; // expected-warning {{definition of a static variable in a constexpr function is incompatible with C++ standards before C++2b}} \
+                          // expected-note {{control flows through the declaration of a static variable}}
+  return m;
+}
+constexpr int g(int n) {        // expected-error {{constexpr function never produces a constant expression}}
+  thread_local const int m = n; // expected-warning {{definition of a thread_local variable in a constexpr function is incompatible with C++ standards before C++2b}} \
----------------
Suggest to remove the `return m;` versions of the tests. They are redundant for the purpose of verifying that "control flows through [ ... ]" is enough for the function to never produce a constant expression.


================
Comment at: clang/test/SemaCXX/constant-expression-cxx2b.cpp:30
+  return m;
+}
+
----------------
Call `j`. Just make it go though the early return.


================
Comment at: clang/test/SemaCXX/constant-expression-cxx2b.cpp:38
+  return m;
+}
+
----------------
Same comment as for `j` above (but for `k`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111400