ogoffart added a comment.
Thanks for your review and i'll try to address the concerns.
I believe tools that really need valid code relies on the diagnostics and bail
out on error. On the other hand, tools that may work on code containing error
do a best effort to work on the remaining AST .
And patches like this one improve the remaining AST, so tools like clang-tidy
will be able to also do valid transformation inside the switch statement, which
could not have been possible when the whole body is gone.
The AST stays "valid" in the sense that all the nodes exist (no nullptr) and so
conform to the expectation of the code. The condition of a switch may now be
an OpaqueValueExpr which should not disturb the matchers.
================
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");
----------------
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.
================
Comment at: lib/Sema/SemaStmt.cpp:669
if (Cond.isInvalid())
- return StmtError();
+ Cond = ConditionResult(
+ *this, nullptr,
----------------
aaron.ballman wrote:
> This makes the condition result valid when it isn't. Users of this condition
> result may expect a valid condition result to return nonnull values when
> calling `get()`, which makes me uncomfortable.
Get return a non-null value.
That's why i'm constructing an OpaqueValueExpr placeholder expression.
The ConditionVar (nullptr in the line bellow) can be null. It is null in valid
code most of the time actually, when one does not declare a new variable in in
condition.
But the result is that users of this condition will get a OpaqueValueExpr when
calling get and should not be disturbed by that as they will just take that as
an expression.
https://reviews.llvm.org/D26350
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
