Page MenuHomePhabricator

[C++20] add Basic consteval specifier
ClosedPublic

Authored by Tyker on May 10 2019, 8:44 AM.

Details

Summary

this revision adds Lexing, Parsing and Basic Semantic for the consteval specifier as specified by http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1073r3.html

with this patch, the consteval specifier is treated as constexpr but can only be applied to function declaration.

Changes:

  • add the consteval keyword.
  • add parsing of consteval specifier for normal declarations and lambdas expressions.
  • add the whether a declaration is constexpr is now represented by and enum everywhere except for variable because they can't be consteval.
  • adapt diagnostic about constexpr to print constexpr or consteval depending on the case.
  • add tests for basic semantic.

Diff Detail

Repository
rL LLVM

Event Timeline

Tyker created this revision.May 10 2019, 8:44 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: cfe-commits. · View Herald Transcript
martong resigned from this revision.May 13 2019, 2:17 AM

ASTImporter.cpp looks good to me.

Tyker updated this revision to Diff 199428.May 14 2019, 7:00 AM
Tyker edited the summary of this revision. (Show Details)

code-gen was adapted to consteval.

i have a question. what C++ code generate a complex value in llvm's representation ?

"complex" in this context is the C99 _Complex, which is supported in C++ as an extension, using the same syntax as C. You can just declare a variable with type _Complex float. If you need to manipulate the real and imaginary parts of a variable, you can use the gcc extension __real__/__imag__, although I don't think you need to.

Thanks for working on this!

Comments on the general approach:

  • We should only evaluate each immediate invocation once (this will become essential once we start supporting reflection -- and particularly operations that mutate the AST -- inside consteval functions).
  • Each time an immediate invocation is formed, you should create a ConstantExpr AST node wrapping that call to reflect that it is a constant.
  • You should change ConstantExpr to store an APValue representing the evaluated value of the expression.

Most of the above should be done as a separate preparatory change; we should be able to remove a lot of the existing ad-hoc caching of evaluated values (eg, for enumerators and the initializers of variables) at the same time.

Please also split this into smaller pieces. If you split off a patch to just add the keyword, parsing, and AST representation for the consteval specifier (but treat it identically to constexpr for constant evaluation purposes), that'd be a good first patch for this feature.

clang/include/clang/AST/Decl.h
2102 ↗(On Diff #199428)

Please rename this to isConstexprSpecified() (compare this to how we model inline, which has similar behavior: it can be explicit, or can be implied by other properties of the function).

2109 ↗(On Diff #199428)

Both the constexpr and consteval specifiers make a function a "constexpr function", so I think this should be called simply isConstexpr; callers that want to distinguish constexpr from consteval can use isConsteval() (or we can add a getConstexprKind() or similar function).

clang/include/clang/AST/DeclCXX.h
2115 ↗(On Diff #199428)

Instead of the three bool flags in a row here (which will make call sites hard to read), consider using an enum, perhaps:

enum class ConstexprSpecifierKind { None, Constexpr, Consteval };
clang/include/clang/Basic/DiagnosticSemaKinds.td
2297–2300 ↗(On Diff #199428)

It would be useful in these diagnostics to explain why the call is required to be constant evaluated; please also use the standard terminology "constant expression" here. (Eg, "call to consteval function %0 is not a constant expression")

2308–2314 ↗(On Diff #199428)

In these diagnostics (including the existing one for tags), we should say "declared constexpr" not "marked constexpr".

clang/include/clang/Basic/TokenKinds.def
390–391 ↗(On Diff #199428)

This and char8_t are both adopted C++2a features now (they'e not just proposals any more); please change the comment to just "C++2a keywords".

clang/include/clang/Sema/DeclSpec.h
404 ↗(On Diff #199428)

I think it would be preferable to track only one location here (for the constexpr / consteval specifier) and store a constexpr specifier kind instead of Constexpr_specified, like we do for the other kinds of specifier for which we allow only one of a set of keywords.

clang/include/clang/Sema/Sema.h
985–986 ↗(On Diff #199428)

An "immediate function context" is also "potentially evaluated"; I think what we want to say here is something like "The current context is "potentially evaluated", but will only ever be constant evaluated; runtime code will never be generated for it. (Eg, the case values of a switch, or the body of a consteval function.)"

4052–4053 ↗(On Diff #199428)

Please don't use the "FunctionName - " style in new code.

4054 ↗(On Diff #199428)

return -> \return

4055–4056 ↗(On Diff #199428)

Drop the "Invalid" from this function name; it doesn't add anything. But I think this is the wrong interface anyway -- I'll get to that later.

clang/lib/AST/ExprConstant.cpp
4474–4475 ↗(On Diff #199428)

We should not need to do this; instead, when the evaluator hits a ConstantExpr node, it should use the already-evaluated value of that node.

clang/lib/Parse/ParseDecl.cpp
2480–2487 ↗(On Diff #199428)

Do not pass random English words into diagnostics; this makes translation impossible. Use a %select or two different diagnostics instead. (Passing in C++ syntax such as constexpr or consteval is OK, since they don't need to be translated.)

clang/lib/Parse/ParseExprCXX.cpp
1073–1075 ↗(On Diff #199428)

It doesn't seem worthwhile to me to support consteval in earlier language modes only for lambdas; if we really want to allow consteval as an extension in earlier language modes, the better approach would be to add a __consteval keyword. But regardless of whether we do that, please remove this.

1303–1305 ↗(On Diff #199428)

Please follow the formatting of the adjacent code.

clang/lib/Sema/DeclSpec.cpp
1326–1332 ↗(On Diff #199428)

This should be handled by SetConstevalSpec instead; please look for calls to BadSpecifier to see how we handle this for other kinds of specifier.

clang/lib/Sema/SemaChecking.cpp
10471 ↗(On Diff #199428)

Are the changes to this file really specific to consteval evaluation? They look more general than that; I think this would fix the evaluation of std::is_constant_evaluated() in any constant-evaluated context that we analyzed. Can this be split out of this patch into a separate change?

clang/lib/Sema/SemaDeclCXX.cpp
640–648 ↗(On Diff #199428)

This will produce diagnostics such as "non-constexpr declaration follows constexpr declaration" for a consteval declaration following a constexpr declaration, which seems misleading since a consteval declaration does declare a constexpr function. I think the order of these two checks should be reversed, so that that case will instead be diagnosed as "consteval declaration follows non-consteval declaration".

6655 ↗(On Diff #199428)

The diagnostic text here is:

error: defaulted definition of <thing> is not constexpr

which might be confusing if the user wrote consteval thing = default;. Maybe change the diagnostic to:

"defaulted definition of <thing> cannot be declared %select{constexpr|consteval}1 because its implicit definition would not be constexpr"

or something like that?

clang/lib/Sema/SemaExpr.cpp
5720–5769 ↗(On Diff #199428)

This checking doesn't make sense: we should be checking that the invocation as a whole is a constant expression, not whether the arguments (evaluated in isolation) are constant expressions. This check should be applied to the Expr representing the call, and should wrap it in a ConstantExpr holding the evaluated value if evaluation succeeds.

13417–13435 ↗(On Diff #199428)

This isn't correct: you can take the address of a consteval function within an immediate invocation. In general, we need to delay this checking until we've finished processing any potentially-enclosing immediate invocations. Trying to capture all of the places where we might form a reference to a consteval function name without forming an immediate invocation is also fragile: you'll miss some, and as Clang is extended, there'll be more cases introduced that you miss.

Here's how I was intending to handle this:

  • Change Sema::MarkFunctionReferenced to take an Expr* instead of a location, and update all of its callers. (We'll need a separate function for marking destructors referenced, because destructor calls typically don't have expressions, but destructors can't be consteval so that's OK.)
  • In Sema::MarkFunctionReferenced, if Func is a consteval function and our current context is not a consteval function, add it to a list on the ExpressionEvaluationContext.
  • When forming an immediate invocation, walk all of its subexpressions and remove them all from the list on the ExpressionEvaluationContext
  • When popping the ExpressionEvaluationContext, diagnose any remaining expressions in its list.

(The first step should be done in a separate patch to keep the diffs smaller and easier to review.)

Tyker updated this revision to Diff 201535.May 27 2019, 8:23 AM
Tyker marked 13 inline comments as done.
Tyker retitled this revision from [C++20] add consteval specifier to [C++20] add Basic consteval specifier.
Tyker edited the summary of this revision. (Show Details)

previous revision was spitted in multiple part as requested
consteval specific semantic and code-gen have been removed from the patch.
other parts:

some remarks weren't fixed because the changes are not present in this patch.

Tyker added inline comments.May 27 2019, 8:24 AM
clang/lib/Sema/SemaChecking.cpp
10471 ↗(On Diff #199428)

this can be splited in and other patch. https://reviews.llvm.org/D62009.

clang/lib/Sema/SemaExpr.cpp
5720–5769 ↗(On Diff #199428)

the idea behind this check was "if the function satisfies the rules of constexpr and all it arguments can be constant evaluated. this function call can be constant evaluated". this was a easy way of checking that the call could be evaluated without evaluating it.

this will be added in later patch after https://reviews.llvm.org/D62399.

13417–13435 ↗(On Diff #199428)

delayed to later patch.

rsmith added a comment.Jun 6 2019, 2:55 PM

Thank you!

clang/include/clang/AST/DeclBase.h
1497 ↗(On Diff #201535)

"kind" -> "Kind"
"Contexpr" -> "constexpr"

clang/lib/AST/DeclPrinter.cpp
615 ↗(On Diff #201535)

The only way a defaulted function can be consteval is if consteval was literally written on the declaration, so I think we should print out the consteval keyword for all consteval functions regardless of whether they're defaulted.

clang/lib/Parse/ParseDecl.cpp
2491 ↗(On Diff #201535)

Should this say which specifier was used? Or do we somehow reject eg. sizeof(consteval int) before we get here?

clang/lib/Parse/ParseExprCXX.cpp
1152–1154 ↗(On Diff #201535)

We should produce a -Wc++17-compat diagnostic similar to this for uses of consteval.

clang/lib/Sema/DeclSpec.cpp
1296–1297 ↗(On Diff #201535)

For consteval we should produce an "incompatible with C++ standards before C++2a" diagnostic, not an "incompatible with C++98" diagnostic.

clang/lib/Sema/SemaDecl.cpp
4300 ↗(On Diff #201535)

Please capitalize this variable name.

6664 ↗(On Diff #201535)

Please consider renaming this diagnostic; err_constexpr_no_declarators doesn't describe the problem here. Maybe err_constexpr_wrong_decl_kind or something?

clang/lib/Sema/SemaDeclCXX.cpp
6655 ↗(On Diff #199428)

I don't think the change here has really addressed the problem. We now say:

error: defaulted definition of <thing> is not consteval

in this case, which doesn't explain what's wrong, and appears to directly contradict what the user wrote. The problem is that the implicit definition would not be constexpr, so we should say that.

Tyker updated this revision to Diff 203551.Jun 7 2019, 7:02 AM
Tyker marked 9 inline comments as done.

fixed requested changes except some i commented upon.

clang/lib/Parse/ParseDecl.cpp
2491 ↗(On Diff #201535)

sizeof(consteval int) was rejected before this point but the diagnostics was bad. so i improved it.

clang/lib/Parse/ParseExprCXX.cpp
1152–1154 ↗(On Diff #201535)

a consteval keyword can only be lexed when we are in C++2a because it is a C++2a keyword so the warning would never fire.

clang/lib/Sema/DeclSpec.cpp
1296–1297 ↗(On Diff #201535)

same as previous comment. the consteval keyword cannot be lexed unless we are in c++2a mode so the warning would never fire.

rsmith accepted this revision.Jun 7 2019, 10:31 AM

Only minor comments remain (other than the -Wc++17-compat warning). In the interest of incremental progress, let's leave the -Wc++17-compat warning for a later patch; feel free to commit this after fixing up the other things.

(If you don't have commit access yet, now would be a good time to request it; please read http://llvm.org/docs/DeveloperPolicy.html for details.)

Thanks again!

clang/include/clang/Basic/DiagnosticSemaKinds.td
1598 ↗(On Diff #203551)

Should this say consteval rather than virtual?

Actually... looks like this new diagnostic is not used, and could just be removed?

7860 ↗(On Diff #203551)

Remove stray "be" here.

clang/lib/AST/ExprConstant.cpp
4561 ↗(On Diff #203551)

The Definition && [...] Definition->isInvalidDecl() case was handled a few lines above; this change appears to have no effect.

clang/lib/Parse/ParseExprCXX.cpp
1152–1154 ↗(On Diff #201535)

The purpose of this warning is to warn about code parsed in C++2a mode that would not be valid in C++17. Try (eg) enabling -Wc++98-compat and building some C++11 code to see what's supposed to happen.

clang/lib/Sema/DeclSpec.cpp
1296–1297 ↗(On Diff #201535)

(See previous reply.)

This revision is now accepted and ready to land.Jun 7 2019, 10:31 AM
Tyker updated this revision to Diff 203725.Jun 9 2019, 12:40 AM
Tyker marked 5 inline comments as done.

fixed requested changes.
also adapted lldb to AST change.
I will commit this when i have access.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2019, 1:53 AM
dyung added a subscriber: dyung.Jun 14 2019, 11:15 AM

Hi, the test cxx2a-consteval.cpp that you added in this commit is failing on the PS4 Windows bot.

http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/9/steps/test/logs/stdio

FAIL: Clang :: SemaCXX/cxx2a-consteval.cpp (13131 of 50204)
******************** TEST 'Clang :: SemaCXX/cxx2a-consteval.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   c:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.obj\bin\clang.exe -cc1 -internal-isystem c:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.obj\lib\clang\9.0.0\include -nostdsysteminc -std=c++2a -fsyntax-only C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\SemaCXX\cxx2a-consteval.cpp -verify
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "c:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.obj\bin\clang.exe" "-cc1" "-internal-isystem" "c:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.obj\lib\clang\9.0.0\include" "-nostdsysteminc" "-std=c++2a" "-fsyntax-only" "C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\SemaCXX\cxx2a-consteval.cpp" "-verify"
# command stderr:
error: 'error' diagnostics expected but not seen: 

  File C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\SemaCXX\cxx2a-consteval.cpp Line 9 (directive at C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\SemaCXX\cxx2a-consteval.cpp:10): cannot combine

  File C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\SemaCXX\cxx2a-consteval.cpp Line 30: destructor cannot be marked consteval

  File C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\SemaCXX\cxx2a-consteval.cpp Line 33: struct cannot be marked consteval

  File C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\SemaCXX\cxx2a-consteval.cpp Line 35: typedef cannot be consteval

  File C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\SemaCXX\cxx2a-consteval.cpp Line 40: consteval can only be used in function declarations

  File C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\SemaCXX\cxx2a-consteval.cpp Line 42: consteval can only be used in function declarations

error: 'error' diagnostics seen but not expected: 

  File C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\SemaCXX\cxx2a-consteval.cpp Line 30: destructor cannot be marked constexpr

  File C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\SemaCXX\cxx2a-consteval.cpp Line 33: struct cannot be marked constexpr

  File C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\SemaCXX\cxx2a-consteval.cpp Line 35: typedef cannot be constexpr

  File C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\SemaCXX\cxx2a-consteval.cpp Line 42: constexpr can only be used in variable and function declarations

error: 'warning' diagnostics seen but not expected: 

  File C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\SemaCXX\cxx2a-consteval.cpp Line 9: duplicate 'constexpr' declaration specifier

11 errors generated.


error: command failed with exit status: 1

Can you take a look?

rnk added a subscriber: rnk.Jun 14 2019, 1:16 PM
rnk added inline comments.
cfe/trunk/include/clang/Sema/DeclSpec.h
366

Please always use unsigned for bitfields to avoid sign extensions under MSVC. I've fixed this in r363450. This makes the test you added pass on Windows (@dyung's comment).