hokein added a comment.
since we're now preserving more invalid code, we should check whether const-evaluator is cable of handling these newly-added invalid case.
I have played around it, it seems that if, do/while, while cases are already handled well. `switch` case need some work:
- the ASTs among different cases (`switch (;)`, `switch(;;)`, `switch(!!;)`) are subtle
- the follow case will result an value-dependent violation, the fix would be to handle value-dependent condition in `EvaluateSwitch` (`clang/lib/AST/ExprConstant.cpp`)
constexpr int s() {
switch(!!) {
}
return 0;
}
void a() {
constexpr int k = s();
}
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:1962
///
/// \returns The parsed condition.
+Sema::ConditionResult
----------------
nit: update the doc comment, though the comment is already stale (missing `CK`).
================
Comment at: clang/lib/Parse/ParseStmt.cpp:1194
SourceLocation Loc,
- Sema::ConditionKind CK,
+ Sema::ConditionKind CK, bool MissingOK,
SourceLocation *LParenLoc,
----------------
sammccall wrote:
> 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?
> 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?
sorry, no action needed.
================
Comment at: clang/test/AST/loop-recovery.cpp:40
+
+ switch(!!!) // expected-error {{expected expression}}
+ int switchBody;
----------------
can you add an init-statement switch case? e.g. `switch (;)`, `switch(;;)`, `switch(!!;)`
================
Comment at: clang/test/Parser/cxx0x-attributes.cpp:155
alignas(4 ns::i; // expected-note {{to match this '('}}
+ // expected-error at -1 {{expected ';' after do/while}}
} // expected-error 2{{expected ')'}} expected-error {{expected expression}}
----------------
This looks like a bogus diagnostic, but I think it is ok, as this is a poor-recovery case for clang already -- IIUC, the do-while statement range claims to the `}` on Line56.
this is a case which can be improved by our bracket-matching repair :)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113752/new/
https://reviews.llvm.org/D113752