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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 3 inline comments as done.
sammccall added inline comments.


================
Comment at: clang/lib/Parse/ParseStmt.cpp:1194
                                        SourceLocation Loc,
-                                       Sema::ConditionKind CK,
+                                       Sema::ConditionKind CK, bool MissingOK,
                                        SourceLocation *LParenLoc,
----------------
hokein wrote:
> 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.
Your understanding is right: previously we were returning a "valid but empty" ConditionResult for a while loop with no condition.

We want functions like ParseParenExprOrCondition to be able to recover in this case, without having them "recover" legitimately missing for loop conditions.

It would be possible to have ParseParenExprOrCondition conditionally recover based on MissingOK, but since we're already using the tristate ConditionResult, using its error state seems cleaner.

> 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.

I'm not sure what you're saying here - what is that a case of?
Is there something you'd like me to do here?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:19149
   if (!SubExpr)
-    return ConditionResult();
 
----------------
hokein wrote:
> 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`.
That's right, MissingOK=false is a better (safer) default.

I've checked all callsites: in most we're passing it explicitly, in one the default is correct (if statement), and in the remaining cases the condition is synthetic and should never be null.


================
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
----------------
hokein wrote:
> file name: loop-recovery.cpp => ast-dump-loop-recovery.cpp
Why? This directory is all tests of the AST, usually that's tested by building and then dumping it. 

An `ast-dump-` prefix makes sense if what we're testing is the AST dumping functionality (e.g. its formatting) rather than *using* that functionality to test the AST itself.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113752