[PATCH] D111400: [Clang][C++2b] P2242R3: Non-literal variables [...] in constexpr
Hubert Tong via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sun Mar 20 08:34:19 PDT 2022
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,
----------------
hubert.reinterpretcast wrote:
> cor3ntin wrote:
> > hubert.reinterpretcast wrote:
> > > This seems to unconditionally error in pre-C++2b modes. For consistency, this should be a `-Wc++2b-extensions` warning.
> > We agreed not have this an extension in earlier modes because a valid c++20 program can sfinea on that. Is it not a direction you agree with anymore>
> I think, now that we have the lambda cases all working, we can be confident that `return false;` in older modes is what this code is responsible for in the SFINAE case.
>
> For handling SFINAE-related cases, there may (eventually) be some restructuring to allow focus on only the template-dependent cases, but C++23 might end up needing to do the checking even for non-dependent cases during constant expression evaluation (because the template definition becomes well-formed due to pending removal of IFNDR).
>
> I've also found indications that Clang generally fails to perform strict constant expression evaluation in relation to instantiations that fail to satisfy the requirements for a constexpr function:
> ```
> template <typename T>
> struct A {
> constexpr int f() {
> T t; // cannot be trivial default initialization until P1331
> // (not DR out of Cologne 2019)
> t = T();
> return 42 + t;
> }
> };
>
> template <const int &N>
> short g(int, char (*)[N] = 0);
>
> template <const int &N>
> long g(void *);
>
> struct Z {
> constexpr Z() {}
> constexpr operator int() { return 0; }
> };
> const int x = A<int>().f();
>
> extern decltype(g<x>(0)) q;
> short q;
> ```
>
Okay, GCC errors for the `NonLiteral` case in C++20 mode. I think that's good enough reason for us to leave Clang doing the same (as the patch currently does).
```
struct NonLiteral {
NonLiteral() {}
};
constexpr auto dependent_var_def_lambda(bool b) {
if (!b)
NonLiteral n;
return 0;
}
```
https://godbolt.org/z/fM9fanxrG
================
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;
+}
----------------
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.
================
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
----------------
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.
================
Comment at: clang/test/SemaCXX/constant-expression-cxx2b.cpp:202-206
+ auto non_literal = [] (bool b) { // cxx2a-note 2{{declared here}}
+ if (!b)
+ NonLiteral n; // cxx2b-note {{non-literal type 'NonLiteral' cannot be used in a constant expression}}
+ return 0;
+ };
----------------
Suggest to go for the version that involves template instantiation:
```
template <typename T>
constexpr auto dependent_var_def_lambda(void) {
return [](bool b) {
if (!b)
T t;
return 0;
};
}
constexpr auto non_literal_valid_in_cxx2b = dependent_var_def_lambda<NonLiteral>()(true);
```
================
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}} \
----------------
Isn't this the "not 'ok'" case? The naming as `non_literal_ok` is confusing.
================
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;
+ };
+
----------------
cor3ntin wrote:
> 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
I was saying that these `test_in_lambdas` tests (not run under C++20 at the time) were mostly redundant.
I agree that we don't need to test the full set of explicitly-`constexpr` lambdas. The "tests should be structured" comment actually asks for less tests (focus on the callable in constant expression evaluation case). That is because the new rules are not used in the implicitly-`constexpr` determination for lambdas in older modes. Contrasting explicitly-`constexpr` lambdas in older modes (which can be evaluated in constant expressions) with the corresponding not-explicitly-`constexpr` lambdas (which are not implicitly `constexpr`) has meaning.
It may help to leave a short rationale comment in the test for why we have one(?) test with an explicitly-`constexpr` lambda and why we only call it for a successful constant expression evaluation.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111400/new/
https://reviews.llvm.org/D111400
More information about the cfe-commits
mailing list