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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits