hubert.reinterpretcast 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;
+}
----------------
cor3ntin wrote:
> 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`
Regarding the test here (and related possible tests), I'm willing to go with
what's in the patch now.
I was pointing out that `constant-expression-cxx2b.cpp` //does// have cases
that are not evaluated (`::f` and `::g`) where the construct is reached
unconditionally (i.e., cases like this one, except not for `goto`); therefore,
this test and those tests should be in the same file.
At the same time, the compat warning should be tested (in this file) as being
present on the definition even if it is only conditionally reachable and there
are no uses of the function (`dtor.cpp` covers the same for the extension
warning).
As for which file tests like the current one should go, I would characterize
the difference between `p3-2b.cpp` and `constant-expression-cxx2b.cpp` as not
"define versus use", but instead as "definition rules versus evaluation rules".
I would argue that the truth of "never produces a constant expression" is a
consequence from the evaluation rules.
================
Comment at: clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3-2b.cpp:32-34
+constexpr void non_literal() { // expected-error {{constexpr function never
produces a constant expression}}
+ NonLiteral n; // expected-note {{non-literal type
'NonLiteral' cannot be used in a constant expression}}
+}
----------------
hubert.reinterpretcast wrote:
> For the reason above, I think this should be in
> `constant-expression-cxx2b.cpp` instead.
If moving tests around re: "never produces a constant expression", move this
one (`non_literal()`) too.
================
Comment at: clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3-2b.cpp:38
+ if (!b)
+ NonLiteral n;
+}
----------------
cor3ntin wrote:
> hubert.reinterpretcast wrote:
> > For consistency, this should warn (under `-Wpre-c++2b-compat`).
> I though we decided *not* to do that
Actually, I think we only decided that it should always be an error in older
modes (and we didn't decide not to add a compat warning). That is, the
extension and compat warnings just happen to be paired currently in the patch.
Now that the code has cleared out of my system a bit, I believe that there is
no fundamental reason for the two warnings to be paired.
I'm fine with getting this patch landed and then fixing this after. Maybe
@aaron.ballman would comment as well.
================
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:
> > 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?
Yes it does. I think that would be good (but does not need to be part of this
patch).
================
Comment at: clang/test/SemaCXX/constant-expression-cxx2b.cpp:118
+
+} // namespace eval_goto
+
----------------
cor3ntin wrote:
> 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?
We're testing that the extension behaviour is actually extended to
explicitly-`constexpr` lambdas in C++20 mode despite the non-application (in
C++20 mode) of the new rules when determining whether a lambda is implicitly
`constexpr`.
Having the test reinforces that the behaviour is as intended, which serves a
design documentation purpose.
The test also arises when applying closed-box testing methodology to speculate
how a desired behaviour may have been implemented in a way that also leads to
undesired behaviour. In other words, maybe the code keyed off too much on being
in a lambda body.
Yes, I know we can read the code, but then again that's today's code and not
necessarily tomorrow's.
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
