aaron.ballman added inline comments.
================
Comment at: clang/lib/Parse/ParseStmt.cpp:1541
+ if (IsConsteval && NotLocation.isValid()) {
+ if (ElseStmt.isUnset())
+ ElseStmt = Actions.ActOnNullStmt(ThenStmtLoc);
----------------
erichkeane wrote:
> 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.
I haven't had the chance to go over this review yet, but this comment caught my
eye in my inbox so I figured I'd respond quickly.
The current approach is definitely clever, but I don't think it's the right way
to tackle this. Generally, the AST needs to retain enough fidelity to be pretty
printed back out to the original source, which wouldn't work here. But also,
this makes it harder to write AST matchers over the construct because it's not
really matching what the user wrote in source (we sometimes get around this by
having a "semantic" and "syntactic" AST representation, but that seems like
overkill here).
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
