aaron.ballman added a comment.

In https://reviews.llvm.org/D26350#809577, @ogoffart wrote:

> The problem i'm trying to solve is precisely to keep as much as possible of 
> the valid AST in the main AST, despite errors.
>  I've already done some work with r249982, r272962 and more, and there is 
> still a lot to do. But the goal is to keep as much as possible of it.


Thank you for the explanation -- I like the goal, but am definitely concerned 
about the assumptions it might break by doing this (in general, not specific to 
this patch). We try to recover gracefully whenever possible, but still, a whole 
lot of frontend code relies on knowing when something is invalid to prevent the 
compiler from doing even worse things. I'm not certain the best balance to 
strike with that, but suspect it's going to adversely impact tools like 
clang-tidy which tend to assume that AST matchers match *valid* code and not 
invalid code.



================
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");
 
----------------
succeed -> succeeds

Are the concerns pointed out in the FIXME addressed by code not in this patch?


================
Comment at: lib/Sema/SemaStmt.cpp:669
   if (Cond.isInvalid())
-    return StmtError();
+    Cond = ConditionResult(
+        *this, nullptr,
----------------
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.


https://reviews.llvm.org/D26350



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to