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