[PATCH] D26350: Keep invalid Switch in the AST

[Olivier Goffart via Phabricator via cfe-commits]

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