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