aaron.ballman added inline comments.

================
Comment at: clang/lib/Parse/ParseStmt.cpp:1541
+  if (IsConsteval && NotLocation.isValid()) {
+    if (ElseStmt.isUnset())
+      ElseStmt = Actions.ActOnNullStmt(ThenStmtLoc);
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > 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).
> This is exactly the wording though. 
> 
> > An if statement of the form if ! consteval compound-statement is not itself 
> > a consteval if statement, but is equivalent to the consteval if statement 
> > `if consteval { } else compound-statement`
>    An if statement of the form `if ! consteval compound-statement1 else 
> statement2` is not itself a consteval if statement, but is equivalent to the 
> consteval if statement
> 
> Doing something else would require storing the not position, and I don't 
> think we gain any functionality?
Sure, the standard also talks about how a `for` loop is equivalent to a `while` 
statement, but we still don't model our AST that way. We gain functionality 
with the pretty printing facilities actually working and being able to AST 
match more naturally (like wanting to match a `not` predicate of an `if 
constexpr` statement).


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

Reply via email to