[PATCH] D110482: [clang] Implement if consteval (P1938)

[Aaron Ballman via Phabricator via cfe-commits]

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;
----------------
aaron.ballman wrote:
> erichkeane wrote:
> > 
> 
Looks like this one was missed -- also, this needs to be `unsigned` instead of using the enum directly (see discussion below).


================
Comment at: clang/include/clang/Basic/Specifiers.h:45
+    Consteval,
+    ConstevalNegated
+  };
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > 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.)
> Mixed with your other suggestion on changing the functions, I think this ends up being a good suggestion.
Sorry, Erich and I probably made this less clear -- can you go with:
```
  enum class IfStatementKind : unsigned {
    Ordinary,
    Constexpr,
    NonnegatedConsteval,
    NegatedConsteval
  };
```
(or flip the word order to `ConstevalNonnegated` if you prefer). This keeps it aligned with the names of the functions on `IfStmt`.


================
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++) {
----------------
aaron.ballman wrote:
> 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)) {
>   ...
> }
> ```
Did this one get missed?


================
Comment at: clang/lib/AST/JSONNodeDumper.cpp:1493
+  attributeOnlyIfTrue("isConsteval", IS->isConsteval());
+  attributeOnlyIfTrue("constevalIsNegated", IS->isConsteval() && IS->isNegatedConsteval());
 }
----------------
aaron.ballman wrote:
> 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?
After discussing with @cor3ntin off-list, I'm retracting this suggestion -- the `OnlyIfTrue` bit means this will be mutually exclusive output already.


================
Comment at: clang/lib/Parse/ParseStmt.cpp:1341-1342
 /// [C++]   'if' '(' condition ')' statement 'else' statement
+/// [C++23] 'if' '!' [opt] consteval statement
+/// [C++23] 'if' '!' [opt] consteval statement 'else' 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();
+    }
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > 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?
> I think the code reads better that way - we need to parse any kind of statement in other cases. In any case the work is about the same
Hmmm, this makes me a bit uncomfortable, but I'm not convinced it's a problem either. Calling `ParseCompoundStmt()` would require checking the token first to avoid an assert, so I can understand better wanting to avoid the messier code that comes from that. However, calling the correct parsing method also means that we don't attempt to parse something we shouldn't parse only to reject it much later; I'm a bit concerned that we'll get some bad error recovery when parsing the more general thing. However, my attempts to come up with test cases where this matters are so far falling pretty flat. One test that would help (and should pass still) is:
```
if consteval ({1;}); // Do we correctly reject the statement expression?
```


================
Comment at: clang/lib/Sema/SemaStmt.cpp:924-926
+      if (FD && FD->isConsteval()) {
+        Immediate = true;
+      }
----------------



================
Comment at: clang/lib/Sema/SemaStmt.cpp:928-930
+    if (isUnevaluatedContext() || Immediate) {
+      Diags.Report(IfLoc, diag::warn_consteval_if_always_true) << Immediate;
+    }
----------------



================
Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:2756-2758
+          /* HasElse=*/Record[ASTStmtReader::NumStmtFields],
+          /* HasVar=*/Record[ASTStmtReader::NumStmtFields + 1],
+          /* HasInit=*/Record[ASTStmtReader::NumStmtFields + 2]);
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > Can you explain these changes? I'm not certain why they were needed.
> I could revert them back actually - I'm just storing the constexpr info after, which seems simpler
Oh, I see what's going on now. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110482/new/

https://reviews.llvm.org/D110482