[PATCH] D110482: [clang] Implement if consteval (P1938)
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 29 06:30:53 PDT 2021
aaron.ballman added inline comments.
================
Comment at: clang/include/clang/AST/Stmt.h:164
- /// True if this if statement is a constexpr if.
- unsigned IsConstexpr : 1;
+ /// Whether this is an if constexpr if or a consteval if or neither.
+ IfStatementKind Kind : 3;
----------------
erichkeane wrote:
>
================
Comment at: clang/include/clang/AST/Stmt.h:2081-2095
+ bool isConsteval() const {
+ return IfStmtBits.Kind == IfStatementKind::Consteval;
+ }
+ bool isConstexpr() const {
+ return IfStmtBits.Kind == IfStatementKind::Constexpr;
+ }
+
----------------
How do folks feel about this? My concern is that `if ! consteval (whatever)` is still a consteval if statement, and we care "is this a consteval if statement" more often than we care which form it is, so it's more helpful that `isConsteval()` returns true for both forms. For the times when we care about negated vs nonnegated, we can use the more explicit functions (which have better parity with their names).
================
Comment at: clang/include/clang/AST/Stmt.h:2122
child_range children() {
- return child_range(getTrailingObjects<Stmt *>(),
+ // We always store a condition, but there is none for if consteval
+ // statements, so skip it.
----------------
erichkeane wrote:
> Not sure if `consteval if` follows the same naming convention as `constexpr if`?
Yup: https://eel.is/c++draft/stmt.if#4.sentence-1
================
Comment at: clang/include/clang/AST/Stmt.h:166
+ /// True if this if statement is a if consteval statement.
+ unsigned IsConsteval : 1;
----------------
erichkeane wrote:
> cor3ntin wrote:
> > erichkeane wrote:
> > > Not sure how others feel here, but for me, I kinda hate that we're using 'unsigned' bitfields for all of this, particularly because these two are mutually exclusive. I'd prefer (though listen to others here first) if the type of this was something more like:
> > >
> > > IfEvalKind EvalKind : 2; // where IfEvalKind is enum class IfEvalKind {None, Constexpr, Consteval};
> > I was not quite sure where to put an enum I could reuse in different place.
> > But I think I'd agree with you otherwise.
> > Any suggestion for where to put it?
> My best suggestion is somewhere in include/Basic.
>
> We don't have a great file I think to fit it, but we DO have a ton where we've created files for smal lthings (see ExceptionSpecificationType.h, Linkage.h, or Lambda.h).
>
> Looking through, there _IS_ a similar enum in Specifiers.h (perhaps the best place for this) that might work well, ConstexprSpecKind might actually just be what we want and could replace the other enum you created later.
> Not sure how others feel here, but for me, I kinda hate that we're using 'unsigned' bitfields for all of this, particularly because these two are mutually exclusive.
Unfortunately I have stronger feelings here. We need the bit-fields to all have the same type, otherwise there's problems with layout in MSVC (also, we often get odd diagnostic behavior we have to come back and fix later, as in https://reviews.llvm.org/rGaf3a4e97d8627a32606ed32001583fe08f15b928). That's why we use `unsigned` for all our bit-fields.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1506
+def warn_if_consteval_always_true : Warning<
+ "if consteval is always true in an %select{unevaluated|immediate}0 context">, InGroup<DiagGroup<"redundant-if-consteval">>;
+
----------------
erichkeane wrote:
>
I think the diagnostic group should be named `redundant-consteval-if`.
================
Comment at: clang/include/clang/Basic/Specifiers.h:45
+ Consteval,
+ ConstevalNegated
+ };
----------------
erichkeane wrote:
> Reads nicer to me maybe? IDK, feel free to ignore.
Alternatively, `ConstevalNonnegated` and `ConstevalNegated` to make it more clear that they're distinguishable from one another. (I don't feel strongly about any particular opinion here.)
================
Comment at: clang/include/clang/Sema/Sema.h:1223
+ /// occurs in an immediate function context - either a consteval function
+ /// or an 'if consteval' function.
+ ImmediateFunctionContext,
----------------
erichkeane wrote:
>
I like the suggestion from @erichkeane, but drop the single quotes as it's no longer talking about syntax at that point.
================
Comment at: clang/include/clang/Sema/Sema.h:9137
+ "Must be in an expression evaluation context");
+ for (auto it = ExprEvalContexts.rbegin(); it != ExprEvalContexts.rend();
+ it++) {
----------------
erichkeane wrote:
> cor3ntin wrote:
> > erichkeane wrote:
> > > What is our iterator type? I THINK at minimum require this to be `auto *`
> > The iterator type is ungodly. Likely `SmallVector<ExpressionEvaluationContextRecord, 8>::reverse_iterator`. I think auto here is more readable but I can change if you want to
> >
> I more questioned due to the lack of the `*`, I see it is the reverse-iterator now, so i don't think that makes it a pointer type, so I don't think it is valuable here. @aaron.ballman is our 'auto nitpicker', so I'll let him to suggest the right answer.
I'd rewrite to avoid the iterators entirely:
```
for (const ExpressionEvaluationContextRecord &Rec : llvm::reverse(ExprEvalContexts)) {
...
}
```
================
Comment at: clang/lib/AST/Interp/ByteCodeStmtGen.cpp:195
+ if (IS->isNegatedConsteval())
+ return IS->getElse();
+
----------------
This probably signals that we're missing some test coverage, but this is the first I've heard of this file so I'm not clear on *how* to test it!
================
Comment at: clang/lib/AST/JSONNodeDumper.cpp:1493
+ attributeOnlyIfTrue("isConsteval", IS->isConsteval());
+ attributeOnlyIfTrue("constevalIsNegated", IS->isConsteval() && IS->isNegatedConsteval());
}
----------------
erichkeane wrote:
> Is the first condition necessary here? I would think isNegatedConsteval should NEVER be true if isConsteval?
All three of these are mutually exclusive, so I think it might be better to have: `kind = Ordinary | constexpr | consteval | negated consteval` as an enumeration rather than having three different fields. WDYT?
================
Comment at: clang/lib/AST/StmtPrinter.cpp:246-249
+ if (If->getElse()) {
+ Indent();
+ OS << "else";
+ PrintStmt(If->getElse());
----------------
================
Comment at: clang/lib/CodeGen/CGStmt.cpp:715-716
void CodeGenFunction::EmitIfStmt(const IfStmt &S) {
+ // The else branch of a consteval statement is always the only branch that can
+ // be runtime evaluated
+ if (S.isConstevalOrNegatedConsteval()) {
----------------
================
Comment at: clang/lib/Parse/ParseStmt.cpp:1341
/// [C++] 'if' '(' condition ')' statement 'else' statement
+/// [C++] 'if' consteval statement 'else' statement
///
----------------
This is not the grammar that was adopted and should be updated accordingly. Also, please clearly mark it as C++2b grammar.
================
Comment at: clang/lib/Parse/ParseStmt.cpp:1449
+ Sema::ExpressionEvaluationContextRecord::EK_Other, ShouldEnter);
ThenStmt = ParseStatement(&InnerStatementTrailingElseLoc);
}
----------------
This looks incorrect for a consteval if -- that requires parsing a compound statement explicitly, not just any statement.
================
Comment at: clang/lib/Parse/ParseStmt.cpp:1520-1525
+ if (IsConsteval) {
+ if (!isa_and_nonnull<class CompoundStmt>(ThenStmt.get())) {
+ Diag(ConstevalLoc, diag::err_expected_after) << "consteval"
+ << "{";
+ return StmtError();
+ }
----------------
Oh. I see where the diagnostic is happening. I think it's unclean to parse a statement and then diagnose it later as being the wrong kind of statement -- why not parse the correct kind of statement initially?
================
Comment at: clang/lib/Sema/SemaStmt.cpp:922-923
+ if (CurContext->isFunctionOrMethod()) {
+ const Decl *D = Decl::castFromDeclContext(CurContext);
+ if (D->getAsFunction() && D->getAsFunction()->isConsteval()) {
+ Immediate = true;
----------------
================
Comment at: clang/lib/Sema/SemaStmt.cpp:929
+ Diags.Report(IfLoc, diag::warn_if_consteval_always_true)
+ << (Immediate ? 1 : 0);
+ }
----------------
Conversions just work this way (http://eel.is/c++draft/conv.integral#2).
================
Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:2756-2758
+ /* HasElse=*/Record[ASTStmtReader::NumStmtFields],
+ /* HasVar=*/Record[ASTStmtReader::NumStmtFields + 1],
+ /* HasInit=*/Record[ASTStmtReader::NumStmtFields + 2]);
----------------
Can you explain these changes? I'm not certain why they were needed.
================
Comment at: clang/test/CXX/stmt.stmt/stmt.select/stmt.if/p4.cpp:148
+}
\ No newline at end of file
----------------
Please add the newline.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110482/new/
https://reviews.llvm.org/D110482
More information about the cfe-commits
mailing list