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

[Aaron Ballman via Phabricator via cfe-commits]

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