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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.


================
Comment at: clang/lib/Parse/ParseStmt.cpp:1541
+  if (IsConsteval && NotLocation.isValid()) {
+    if (ElseStmt.isUnset())
+      ElseStmt = Actions.ActOnNullStmt(ThenStmtLoc);
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110482