rsmith added inline comments.
================
Comment at: lib/Parse/ParseStmt.cpp:1297
- if (Switch.isInvalid()) {
- // Skip the switch body.
- // FIXME: This is not optimal recovery, but parsing the body is more
- // dangerous due to the presence of case and default statements, which
- // will have no place to connect back with the switch.
- if (Tok.is(tok::l_brace)) {
- ConsumeBrace();
- SkipUntil(tok::r_brace);
- } else
- SkipUntil(tok::semi);
- return Switch;
- }
+ assert(!Switch.isInvalid() && "ActOnStartOfSwitchStmt always succeed");
----------------
ogoffart wrote:
> aaron.ballman wrote:
> > succeed -> succeeds
> >
> > Are the concerns pointed out in the FIXME addressed by code not in this
> > patch?
> The FIXME is pointing out problems occuring if the parser found 'default' or
> 'case' statement, but cannot connect it to the corresponding 'switch'
> statement (because that switch statement did not exist as it was removed from
> the AST).
>
> Now that we always keep the 'switch' statement, this is no longer a problem.
I'm uncomfortable about this; this change couples Parser to the implementation
details of Sema. How about this: remove this assert and change
`ActOnFinishSwitchStmt` to take a `StmtResult` instead (which might be
invalid). Then you can tell from within `ActOnFinishSwitchStmt` whether to
check the case statements against the condition based on whether the switch is
in fact invalid.
(Alternatively: change `ActOnStartSwitchStmt` to return `void` and make
`ActOnFinishSwitchStmt` pick up the switch statement from the `SwitchStack`.)
================
Comment at: lib/Sema/SemaStmt.cpp:672
+ MakeFullExpr(new (Context) OpaqueValueExpr(SourceLocation(),
+ Context.IntTy, VK_RValue),
+ SwitchLoc),
----------------
Won't this result in warnings or errors later on if we have `case` labels with
expressions of other types? (Eg, narrowing warnings/errors)
Please instead (somehow) track that the switch condition is invalid and skip
those checks -- perhaps either by returning an invalid-but-not-null statement
here and passing that back into `ActOnFinishSwitchStmt`, or by tracking an
"invalid" flag on the `SwitchStack` entry.
================
Comment at: lib/Sema/SemaStmt.cpp:1173-1177
- // FIXME: If the case list was broken is some way, we don't have a good
system
- // to patch it up. Instead, just return the whole substmt as broken.
- if (CaseListIsErroneous)
- return StmtError();
-
----------------
Hmm. Removing this will result in us producing invalid ASTs in some cases (with
duplicate `case` or `default` labels). That's a condition that it would be
reasonable for AST consumers to assert on currently, so this is concerning.
That said... it's inevitable that this work to keep more invalid constructs in
the AST will result in such changes. Perhaps what we need is just a marker to
say "beyond this point the AST does not necessarily correspond to any valid
source code" for `Stmt` nodes, analogous to the `Invalid` marker on
declarations. (Maybe a wrapper `InvalidStmt` node, so that tree traversals can
easily avoid walking through it.)
Let's try this change out as-is. It may be that this concern is baseless.
https://reviews.llvm.org/D26350
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
