[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:
> 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).
Yep, I realized I need to change.
Oh well.


================
Comment at: clang/test/CodeGenCXX/cxx2b-consteval-if.cpp:1
+// RUN: %clang_cc1 -std=c++2b %s -emit-llvm -o - | FileCheck %s --implicit-check-not=should_not_be_used
+
----------------
erichkeane wrote:
> I tend to like the 'CHECK' lines being inline with the code, it makes it easier to follow in these cases.  Additionally, I think the check-lines should be more specific (that they are actually checking for 'call' instructions).
To be honest, I adapted the code from the equivalent test for `if constexpr`, and I don't actually know how it work


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110482