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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/lib/AST/Stmt.cpp:915
 
-IfStmt::IfStmt(const ASTContext &Ctx, SourceLocation IL, bool IsConstexpr,
-               Stmt *Init, VarDecl *Var, Expr *Cond, SourceLocation LPL,
-               SourceLocation RPL, Stmt *Then, SourceLocation EL, Stmt *Else)
+IfStmt::IfStmt(const ASTContext &Ctx, SourceLocation IL, ConstexprSpecKind ConstexprKind, Stmt *Init, VarDecl *Var, Expr *Cond,
+               SourceLocation LPL, SourceLocation RPL, Stmt *Then,
----------------
Does this need clang-format again?


================
Comment at: clang/lib/AST/Stmt.cpp:927
 
+  assert((!IsConsteval && !IsConstexpr) || IsConsteval != IsConstexpr);
+
----------------
erichkeane wrote:
> Oh dear... I'm not quite sure I want this as a scoped enum.
I see I mistyped here, i'm 'NOW' quite sure.  Thanks for fixing it anyway.


================
Comment at: clang/lib/AST/StmtPrinter.cpp:239
 void StmtPrinter::PrintRawIfStmt(IfStmt *If) {
+  if (If->isConsteval()) {
+    OS << "if consteval";
----------------
I think I'm not a fan of the inversion mostly for this, the fact that we end up printing the reverse of what happens in 'if constexpr' is giving me some heartburn.


================
Comment at: clang/lib/Analysis/CFG.cpp:3050
   BinaryOperator *Cond =
-      I->getConditionVariable()
+      I->isConsteval() || I->getConditionVariable()
           ? nullptr
----------------
I think there is a ternary-confuse-warning in GCC that fires sometimes if you skip outer-parens, you might find it necessary/a good idea to put parens around the condition.


================
Comment at: clang/lib/Analysis/CFG.cpp:3064
     // See if this is a known constant.
-    const TryResult &KnownVal = tryEvaluateBool(I->getCond());
+    TryResult KnownVal;
+    if (!I->isConsteval())
----------------
Note for future-reviewer: This ends up being an improvement here, TryResult contains only a single integer in a VERRY bizarre way, it seems to be intended to essentially be a 'true/false/unknown' bit of hackery (despite having 3 states, its an integer?).  If someone runs across this comment in the future and wants an 'easy win', I suspect swapping it with a `llvm::Optional<bool>` or something would be at least an interface-win.


================
Comment at: clang/lib/Parse/ParseStmt.cpp:1541
+  if (IsConsteval && NotLocation.isValid()) {
+    if (ElseStmt.isUnset())
+      ElseStmt = Actions.ActOnNullStmt(ThenStmtLoc);
----------------
So this is interesting.  I'm not sure how I feel about having the AST not represent the textual representation like this.  I'm interested what others have to say.

My understanding is that this makes:

`if consteval { thenstmt; } else { elsestmt;`
be represented as:
`IfStmt isConsteval, with getThen()== thenstmt`

however
`if not consteval { thenstmt; } else { elsestmt;}`
be represented as:
`IfStmt isConsteval, with getThen()== elsestmt`

IMO, this feels too clever.  

I think I'd prefer that the IfStmt know whether it is a 'not consteval' and select the right one that way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110482