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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
