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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
