[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/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