erichkeane 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).
I don't think you'd necessarily have to store the 'not' position, just the
'not-ness'
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110482/new/
https://reviews.llvm.org/D110482
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
