This is an archive of the discontinued LLVM Phabricator instance.

[Clang][C++2b] P2242R3: Non-literal variables [...] in constexpr
ClosedPublic

Authored by cor3ntin on Oct 8 2021, 4:49 AM.

Details

Summary

Allow goto, labelled statements as well as static, thread_local, and
non-literal variables in constexpr functions.

As specified. for all of the above (except labelled statements) constant
evaluation of the construct still fails.

For constexpr bodies, the proposal is implemented with diagnostics as
a language extension in older language modes. For determination of
whether a lambda body satisfies the requirements for a constexpr
function, the proposal is implemented only in C++2b mode to retain the
semantics of older modes for programs conforming to them.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

@aaron.ballman, I haven't had a chance to look at the code in detail yet, but I would like to get on the same page about the error/warning behaviour.

In the older modes, we stay conforming either

  • by producing the warning (when we know the user said they wanted the function to be constexpr) and letting the code through or
  • by doing what the older document says because a warning is inappropriate at the point of the check and we have no mechanism developed (including for other similar cases) for producing the warning only when semantically significant later.

In C++2b, the -Wc++20-compat warning is produced for the places where we warn above and is not produced for the places where we don't warn above.

Considering that the warning would be something like "this function is not implicitly constexpr in C++20 but is so in C++23", and we probably will have a lot of such cases for other reasons, I can buy that people are not interested in such a warning.

clang/test/CXX/basic/basic.types/p10.cpp
23 ↗(On Diff #400325)

Seems like the only change in this file is drive-by NFC. Can this be pulled out of this patch?

In C++2b, the -Wc++20-compat warning is produced for the places where we warn above and is not produced for the places where we don't warn above.

I'm not seeing any tests looking for these "incompatible with" messages.

cor3ntin updated this revision to Diff 414880.Mar 12 2022, 1:31 PM
  • Rebase
  • Update cxx_status as we are now targeting clang 15
  • Test the pre-C++2b warnings

Noting for myself:
Clang's status quo already has behaviours that are similar to P2242R3 in its C++20 mode despite those behaviours being non-conforming and contributing to binary compat breakage with GCC.
This patch is not responsible for those behaviours, and fixing that status quo is not within the scope of this patch.

Shorter test:

struct NonLit {
  NonLit();
};
template <typename T>
constexpr int foo() {
  return 42;
  T t;
}
int (*f())() {
  const int x = foo<NonLit>();
  auto ff = [] { return x; }; // C++20 should error here; x is odr-used and not captured
  return ff;
}
clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3-2b.cpp
17

Add a NonLiteral case and a case with a labelled statement and no goto?

38–39

I'm mildly confused over why the error case for the constexpr evaluation appears in this file (which is about the structure of the body) and not in constant-expression-cxx2b.cpp (which is about constant expression evaluation).

clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp
196–203

The return statement needs to happen before the declaration of the static variable.

354

The abbreviated message snippet might be easy to understand if someone was looking at the diff from this patch, but it would likely be less so if they were just looking at the code later.

clang/test/SemaCXX/constant-expression-cxx2b.cpp
4–8

Same comment as the thread_local case below. Also, the expected error is missing even if the function is called in a context requiring constant evaluation.

9–13

The implementation generates an error because the return is m and not something like &m - &m.

The wording has:

  • a control flow that passes through a declaration of a variable with static or thread storage duration

as one of the things that cannot be evaluated for a constant expression.

The patch does not generate an error on the &m - &m version of the test.

clang/test/SemaCXX/cxx1z-constexpr-lambdas.cpp
118–122 ↗(On Diff #414880)

Patch does not change the behaviour of this test anymore (and this file does not have a C++2b RUN line). Should keep the test.

cor3ntin updated this revision to Diff 415041.Mar 14 2022, 2:56 AM
cor3ntin marked 2 inline comments as done.

Address Hubert's feedback

  • Add a diagnostic during constant evaluation when evaluating the declaration of a variable with non-automatic storage duration
  • Add more tests
  • Restore incorrect removed test (not sure what happened there!)
  • Fix some tests
  • Reorganize some tests to be in the correct file
cor3ntin marked 4 inline comments as done.Mar 14 2022, 2:56 AM
cor3ntin updated this revision to Diff 415240.Mar 14 2022, 2:52 PM

clamg-format

cor3ntin updated this revision to Diff 415353.Mar 15 2022, 1:22 AM

Fix test messed up by automatic formatting.

aaron.ballman added inline comments.Mar 15 2022, 7:18 AM
clang/lib/AST/ExprConstant.cpp
5128–5130
5173–5176
clang/lib/Sema/SemaDeclCXX.cpp
2130
2132–2136
clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/dtor.cpp
1

I think we want to keep testing the C++20 behavior and want new tests for the C++2b behavior. We still need to be sure we're correct in C++20 mode given the differences (even in extensions because -pedantic-errors is a thing). So I'd recommend splitting this test into two or using an additional RUN line.

clang/test/SemaCXX/constant-expression-cxx14.cpp
47

I think we should keep this test coverage by adding -Werror=c++2b-extensions to the RUN line for C++2b.

71
clang/test/SemaCXX/constant-expression-cxx2b.cpp
9–10
61–62

This error seems suspect to me. If we made flowing through a thread_local into an extension (line 54), then this code should be accepted. However, I think we're getting the error because the constant expression evaluator produces the note on line 55 and that usually will cause the evaluator to claim it wasn't a constant expression.

78
cor3ntin updated this revision to Diff 415425.Mar 15 2022, 7:33 AM
cor3ntin marked 6 inline comments as done.

Address formatting and style issues

clang/lib/Sema/SemaDeclCXX.cpp
2132–2136

This is consistent with the same code above. Should I be inconsistent?

aaron.ballman added inline comments.Mar 15 2022, 7:35 AM
clang/lib/Sema/SemaDeclCXX.cpp
2132–2136

I don't have strong opinions, either way is defensible. I just have a hard time reading the code with nesting the substatements like this.

cor3ntin added inline comments.Mar 15 2022, 7:37 AM
clang/test/SemaCXX/constant-expression-cxx2b.cpp
61–62

We can not evaluate at compile time a thread_local or static, even if we allow them to appear in a constexpr function. So this is the behavior I'd expect

cor3ntin added inline comments.Mar 15 2022, 7:40 AM
clang/lib/Sema/SemaDeclCXX.cpp
2132–2136

Would you be happy if i fix all of them later as a nfc?

aaron.ballman added inline comments.Mar 15 2022, 7:42 AM
clang/lib/Sema/SemaDeclCXX.cpp
2132–2136

Absolutely! Or if you want to land an NFC fix now and rebase your patch on top of that, that'd also be fine.

clang/test/SemaCXX/constant-expression-cxx2b.cpp
61–62

Yes, you're right, I lost that context before. Sorry for the noise, I think the error here is correct. But I also worry that users may get confused in the same way I just did. That said, I can't think of a better diagnostic wording and I think the behavior in situ will be fine because the note will be "attached" to the error in the diagnostic output, which makes the error more clear than reading the code statically in the review.

cor3ntin updated this revision to Diff 415438.Mar 15 2022, 8:02 AM

Restore the C++14 tests

@aaron.ballman @cor3ntin, are we confident that testing the non-lambda cases is sufficient to cover the lambda cases as well?

I suggest using a pattern such as:

int (*test_cxx2b_constexpr_label_in_body())() {
  auto qq = []() {
    label: return 42;
  };
  const int x = qq();
  auto ff = [] { return x; }; // passes in C++2b; error in C++20
  return ff;
}

For each of the cases.

clang/lib/AST/ExprConstant.cpp
5010

I don't see anything in the wording exempting constexpr static (although I suppose it makes sense to, but GCC does not make such an exemption in its C++2b mode). @cor3ntin, can you open a discussion on the reflector?

@aaron.ballman @cor3ntin, are we confident that testing the non-lambda cases is sufficient to cover the lambda cases as well?

I think lambdas are just odd enough that they'd be worth testing independently.

I suggest using a pattern such as:

int (*test_cxx2b_constexpr_label_in_body())() {
  auto qq = []() {
    label: return 42;
  };
  const int x = qq();
  auto ff = [] { return x; }; // passes in C++2b; error in C++20
  return ff;
}

For each of the cases.

Not a bad way to test that!

cor3ntin updated this revision to Diff 415573.Mar 15 2022, 2:05 PM
  • Add tests in lambdas
  • Do not allow static constexpr:

I can't think of a scenario in which that would
be problematic *today*, but I agree it would be
non-conforming and should be discussed.
For example it would affect https://wg21.link/p0596.

I removed that, and added tests to check it is correctly
ill-formed.

cor3ntin updated this revision to Diff 415574.Mar 15 2022, 2:06 PM

Formatting

clang/test/SemaCXX/constant-expression-cxx2b.cpp
102

I think a lambda marked constexpr and one that isn't has fundamental differences in behaviour in relation to this patch. As noted in previous comments, the constexpr case warns and then allows the lambda to be called in constant evaluation. The other case produces no message for the lambda definition itself and does not allow the lambda to be called in a constexpr context (even if the control flow is accepted by C++2b).

cor3ntin updated this revision to Diff 415719.Mar 16 2022, 12:53 AM

Add more tests for implicitely constexpr lambdas

cor3ntin updated this revision to Diff 415720.Mar 16 2022, 12:55 AM

Formatting

@hubert.reinterpretcast I added the tests, please let me know what you think. Thanks!

hubert.reinterpretcast retitled this revision from [Clang] Implement P2242R3 to [Clang][C++2b] P2242R3: Non-literal variables [...] in constexpr.Mar 17 2022, 4:54 PM
hubert.reinterpretcast edited the summary of this revision. (Show Details)
clang/lib/Sema/SemaDeclCXX.cpp
1904–1906

This seems to unconditionally error in pre-C++2b modes. For consistency, this should be a -Wc++2b-extensions warning.

clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/dtor.cpp
1

@cor3ntin, I don't think Aaron's comment has been addressed. I think keeping -std=c++2a and adding -Wc++2b-extensions would be appropriate. I know there are tests elsewhere that also focus on paragraph 3, but this test has coverage specifically for destructors.

clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3-2b.cpp
39

For consistency, this should warn (under -Wpre-c++2b-compat).

clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp
1–5

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.

clang/test/SemaCXX/constant-expression-cxx14.cpp
47

@aaron.ballman, the tests have been restored; however, they test the extension behaviour now. The -Werror=c++2b-extensions version of the tests are covered by the tests elsewhere that focus on [dcl.constexpr] paragraph 3.

clang/test/SemaCXX/constant-expression-cxx2b.cpp
4–14

Suggest to remove the return m; versions of the tests. They are redundant for the purpose of verifying that "control flows through [ ... ]" is enough for the function to never produce a constant expression.

31

Call j. Just make it go though the early return.

39

Same comment as for j above (but for k).

cor3ntin added inline comments.Mar 19 2022, 12:13 AM
clang/lib/Sema/SemaDeclCXX.cpp
1904–1906

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>

clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3-2b.cpp
39

I though we decided *not* to do that

cor3ntin updated this revision to Diff 416672.Mar 19 2022, 1:00 AM
cor3ntin marked 2 inline comments as done.
  • Test dcl.constexpr/dtor.cpp in both C++20 and C++23 modes
  • Make sure j/k are actually called in constant-expression-cxx2b.cpp
cor3ntin marked 2 inline comments as done.Mar 19 2022, 1:01 AM
cor3ntin marked 2 inline comments as done.
cor3ntin added inline comments.
clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/dtor.cpp
1

Yes, I missed that, thanks

cor3ntin marked an inline comment as done.Mar 19 2022, 1:06 AM
cor3ntin added inline comments.
clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp
1–5

This seems inconsistent with how that file is currently structured though

I think labels can be left as is.

Yes, the static, thread_local, label, and goto cases would all categorically contribute to a IFNDR program in the older modes (and we currently reliably diagnose them in the definition context).

clang/lib/Sema/SemaDeclCXX.cpp
1904–1906

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;
clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp
1–5

I meant to change the entire file to check for warnings instead of errors. I guess that really should be a separate patch.

clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3-2b.cpp
18–23

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.

33–35

For the reason above, I think this should be in constant-expression-cxx2b.cpp instead.

clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp
1–5

I guess the change will cause the "never produces a constant expression" warning unless if that is suppressed with -Wno-invalid-constexpr.

clang/test/SemaCXX/constant-expression-cxx2b.cpp
96

I see no tests in the entire patch where a labelled statement is evaluated successfully in a constant expression evaluation. This file will be where such a test should be.

97–124

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.

126–155

It is quite important that all of the cases (label, goto, static, thread_local, and non-literal) is tested in lambdas not explicitly constexpr and run in C++20 mode too. The focus should be that:

  • the cases that are implicitly constexpr only in C++2b are not usable in constant expression evaluation in C++20 mode, and
  • they are usable in constant expression evaluation in C++2b mode.

I suggest a separate file for this.

clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/dtor.cpp
26–28

s/becaus/because/;

clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3-2b.cpp
37–40

There should be a version of this in constant-expression-cxx2b.cpp where the function is called successfully during constant expression evaluation.

cor3ntin updated this revision to Diff 416758.Mar 20 2022, 12:31 AM
cor3ntin marked 2 inline comments as done.
  • fix typos
  • add tests for non-literals in lambdas
  • run lambda tests in c++20 mode
cor3ntin updated this revision to Diff 416760.Mar 20 2022, 12:38 AM

Add a test for evaluated labels

cor3ntin added inline comments.Mar 20 2022, 12:39 AM
clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3-2b.cpp
17

These tests exist in that same file

18–23

I'm not sure I understand. No call is made in that file.

clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp
1–5

Yes, we could cleanup this entire file to get rid of the #ifdef, then change how warnings are diagnosed.

clang/test/SemaCXX/constant-expression-cxx2b.cpp
97–124

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

clang/lib/Sema/SemaDeclCXX.cpp
1904–1906

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

clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3-2b.cpp
18–23

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.

clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp
1–5

I am not in favour of getting rid of the #ifdefs. They still tell us that the "conformance" warnings are associated with the right modes.

clang/test/SemaCXX/constant-expression-cxx2b.cpp
97–124

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.

203–207

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);
209

Isn't this the "not 'ok'" case? The naming as non_literal_ok is confusing.

cor3ntin updated this revision to Diff 416792.Mar 20 2022, 10:36 AM
cor3ntin marked 5 inline comments as done.
  • Tidy up tests
  • Add comments
  • Add a test with a non literal variable in dependant context as per Hubert's suggestion
cor3ntin updated this revision to Diff 416793.Mar 20 2022, 10:38 AM
  • clang-format
clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/dtor.cpp
26–28

Typo still present.

clang/test/SemaCXX/constant-expression-cxx2b.cpp
2–3

Flip cxx2a and cxx2b prefixes to match mode used. Add -Wno-c++2b-extensions since tests for those warnings are covered in p3-2b.cpp.

9

It's customary to have the L suffix in the context of checking __cplusplus.

91

Evaluate as constexpr.

117

Evaluate as constexpr.

119

Move #endif to here (from below) so the explicitly-constexpr lambda cases are also tried in C++20 mode.

121–122

Fix typos.

126

Adjust for suggested changes: do not expect C++2b mode warning in C++20 mode.

133

Adjust for suggested changes: do not expect C++2b mode warning in C++20 mode.

142

Adjust for suggested changes: do not expect C++2b mode warning in C++20 mode.

148–149

Minor: Adjust alignment of comments.

153

Adjust for C++20/C++2b differences.

157–159

Guard calls to be C++2b only (since definition is not okay in C++20 mode). Adjust naming so that "ok" name is used for the C++2b-valid case.

161–162

Move #endif up so the explicitly-constexpr lambda cases are also tried in C++20 mode.

clang/test/SemaCXX/constant-expression-cxx2b.cpp
164

Fix typo and expand comment.

166–170

Remove this lambda. It is not callable in any constant expression even under C++2b mode.

195–196

Minor nit: Align comments.

208–210

Minor nit: Align comments.

cor3ntin updated this revision to Diff 416835.Mar 21 2022, 1:11 AM
cor3ntin marked 11 inline comments as done.

Tidy up the tests again

cor3ntin updated this revision to Diff 416838.Mar 21 2022, 1:16 AM

Add ifdef around tests that should not run in c++20 mode

cor3ntin updated this revision to Diff 416839.Mar 21 2022, 1:18 AM

Switch variable names (non_literal_ko/non_literal_ok)

LGTM with minor nit. Thank you.

clang/test/SemaCXX/constant-expression-cxx2b.cpp
5

Minor nit: Formatting.

This revision is now accepted and ready to land.Mar 21 2022, 1:13 PM
cor3ntin updated this revision to Diff 417084.Mar 21 2022, 1:50 PM

Fix formatting

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

clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3-2b.cpp
18–23

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

clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp
1–5

To be clear, i meant using //cxx20-warning and things like that instead, which is functionally equivalent. Does that make sense?

clang/test/SemaCXX/constant-expression-cxx2b.cpp
119

I mean sure I can do that, but what are we testing here?

209

You are right, I switched it.

clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3-2b.cpp
18–23

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.

33–35

If moving tests around re: "never produces a constant expression", move this one (non_literal()) too.

39

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.

clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp
1–5

Yes it does. I think that would be good (but does not need to be part of this patch).

clang/test/SemaCXX/constant-expression-cxx2b.cpp
119

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.

1 request for a test, and 1 hope that @aaron.ballman will help bikeshed (or just say its fine!), but otherwise LGTM.

clang/include/clang/Basic/DiagnosticASTKinds.td
69

Hmm... this feels a little awkward to me. Though, I usually have Aaron do the bikeshedding, so if he didn't come up with better I guess we can let it go.

clang/lib/AST/ExprConstant.cpp
5010

Can we make sure we have tests for the 'other' two types of TSCSpec? I'm not too worried about C11 thread local (it does, after all, require static), but the GNU __thread does not.

aaron.ballman accepted this revision.Mar 22 2022, 6:23 AM

I don't spot anything overly concerning in this patch, I believe it LGTM as well.

clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3-2b.cpp
39

I would also be fine with addressing that after this patch lands.

clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp
1–5

FWIW, I also agree that it'd be good to use -verify=something instead of #ifdef (but not strictly necessary for this patch).

clang/test/SemaCXX/constant-expression-cxx2b.cpp
119

Having the test reinforces that the behaviour is as intended, which serves a design documentation purpose.

We do this in our tests somewhat regularly, but please be sure to add comments to explain that the test is serving a design documentation purpose, otherwise it can be easy to later go "oh, this is obviously broken nonsense so it's fine that behavior changed" by accident.

aaron.ballman added inline comments.Mar 22 2022, 6:25 AM
clang/include/clang/Basic/DiagnosticASTKinds.td
69

I didn't think it was overly awkward, about the only question I had was "should we quote the static and thread_local as keywords?" when I reviewed.

erichkeane added inline comments.Mar 22 2022, 6:27 AM
clang/include/clang/Basic/DiagnosticASTKinds.td
69

Ok, I can live with that. AS far as quoting, I think we are consistently inconsistent on that one.

clang/include/clang/Basic/DiagnosticASTKinds.td
69

Minor nit:
The code already behaves as-if the resolution of CWG 2552 is applied. Let's use the corresponding wording.

cor3ntin updated this revision to Diff 417320.Mar 22 2022, 9:21 AM
  • Add tests for _Thread_local, thread

(these should be supported in c++23 constexpr function)

  • Change /declaration/definition/ in a diagnostic message
erichkeane accepted this revision.Mar 22 2022, 9:35 AM
  • Add tests for _Thread_local, thread

(these should be supported in c++23 constexpr function)

  • Change /declaration/definition/ in a diagnostic message

SGTM, thanks!

I will probably land that soon if there are no further issue

According to my notes, i then need to

  • Cleanup clang/test/SemaCXX/constant-expression-cxx14.cpp (remove -Werror, #ifdef)
  • Add braces in CheckConstexprFunctionStmt
  • We may still miss a compatibility warning for non-literal in c++23 mode?

Confirming LGTM with minor comments.

  • We may still miss a compatibility warning for non-literal in c++23 mode?

My understanding is that Aaron and I both want the warning added (but are okay with having this patch landed first).

clang/test/SemaCXX/constant-expression-cxx2b.cpp
24

Formatting nit.

30

Formatting nit.

Confirming LGTM with minor comments.

  • We may still miss a compatibility warning for non-literal in c++23 mode?

My understanding is that Aaron and I both want the warning added (but are okay with having this patch landed first).

+1 to this

cor3ntin updated this revision to Diff 417341.Mar 22 2022, 11:03 AM

Formatting

@hubert.reinterpretcast @aaron.ballman @erichkeane Thanks a lot for the review. I'm working on making PRs for the further changes we agreed to do.