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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
