[PATCH] D113752: [Parse] Use empty RecoveryExpr when if/while/do/switch conditions fail to parse

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

this looks great, some nits and questions.



================
Comment at: clang/lib/Parse/ParseStmt.cpp:1194
                                        SourceLocation Loc,
-                                       Sema::ConditionKind CK,
+                                       Sema::ConditionKind CK, bool MissingOK,
                                        SourceLocation *LParenLoc,
----------------
what's the purpose of introducing `MissingOK`? It seems unclear to me, my understanding is

- before the patch, ActionOnCondition always returned a valid ConditionResult for an empty ConditionResult. I assume this was conceptually a "MissingOK-true" case.
- now in the patch, the MissingOK is propagated to `ActOnCondition`, which determines the returning result;  ​The MissingOK is set to false in ParseSwitchStatement, ParseDoStatement below. 

The only case I can think of is that we might fail to create a recoveryExpr (when the recovery-ast flag is off) for the condition.


================
Comment at: clang/lib/Parse/ParseStmt.cpp:1811
 
-  if (Cond.isInvalid() || Body.isInvalid())
+  if (Body.isInvalid())
     return StmtError();
----------------
we should keep the `Cond.isInvalid()` branch, because the above typo-correction code path (Line1800) can hit it.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:19149
   if (!SubExpr)
-    return ConditionResult();
+    return ConditionResult(/*Invalid=*/!MissingOK);
 
----------------
nit: `return MissingOK? ConditionResult() : ConditionError()`, is easier to understand.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:19149
   if (!SubExpr)
-    return ConditionResult();
 
----------------
hokein wrote:
> nit: `return MissingOK? ConditionResult() : ConditionError()`, is easier to understand.
while here, we always return a valid condition (conceptually MissingOK is true), but in the patch, the default value of `MissingOK` parameter is false, this seems we are changing the default behavior for all call sides of `ActOnCondition`.


================
Comment at: clang/test/AST/loop-recovery.cpp:1
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: not %clang_cc1 -fsyntax-only -ast-dump %s | FileCheck %s
----------------
file name: loop-recovery.cpp => ast-dump-loop-recovery.cpp


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113752/new/

https://reviews.llvm.org/D113752