yronglin marked 3 inline comments as done.
yronglin added inline comments.
================
Comment at: clang/lib/AST/ExprConstant.cpp:4921
+ // value is.
+ if (isa<RecoveryExpr>(E))
+ return false;
----------------
aaron.ballman wrote:
> yronglin wrote:
> > hokein wrote:
> > > The constant evaluator is not aware of the "error" concept, it is only
> > > aware of value-dependent -- the general idea behind is that we treat the
> > > dependent-on-error and dependent-on-template-parameters cases the same,
> > > they are potentially constant (if we see an expression contains errors,
> > > it could be constant depending on how the error is resolved), this will
> > > give us nice recovery and avoid bogus following diagnostics.
> > >
> > > So, a `RecoveryExpr` should not result in failure when checking for a
> > > potential constant expression.
> > >
> > > I think the right fix is to remove the conditional check `if
> > > (!EvaluateDependentExpr(SS->getCond(), Info))` in `EvaluateSwitch`, and
> > > return `ESR_Failed` unconditionally (we don't know its value, any
> > > switch-case anwser will be wrong in some cases). We already do this for
> > > return-statment, do-statement etc.
> > >
> > >
> > Do you mean?
> > ```
> > if (SS->getCond()->isValueDependent()) {
> > EvaluateDependentExpr(SS->getCond(), Info);
> > return ESR_Failed;
> > }
> > ```
> > the general idea behind is that we treat the dependent-on-error and
> > dependent-on-template-parameters cases the same, they are potentially
> > constant (if we see an expression contains errors, it could be constant
> > depending on how the error is resolved), this will give us nice recovery
> > and avoid bogus following diagnostics.
>
> I could use some further education on why this is the correct approach. For a
> dependent-on-template-parameters case, this makes sense -- either the
> template will be instantiated (at which point we'll know if it's a constant
> expression) or it won't be (at which point it's constant expression-ness
> doesn't matter). But for error recovery, we will *never* get a valid constant
> expression.
>
> I worry about the performance overhead of continuing on with constant
> expression evaluation in the error case. We use these code paths not only to
> get a value but to say "is this a constant expression at all?".
>
> I don't see why the fix should be localized to just the switch statement
> condition; it seems like *any* attempt to get a dependent value from an error
> recovery expression is a point at which we can definitively say "this is not
> a constant expression" and move on.
I understand that continuing to perform constant evaluation when an error
occurs can bring more additional diagnostic information (such as jumping to the
default branch to continue calculation when the condition expression evaluation
of switch-statement fails), but the additional diagnostic message that is
emitted is in some cases doesn't usually useful, and as Aaron said may affect
performance of clang. I don't have enough experience to make a tradeoff between
the two. BTW
https://github.com/llvm/llvm-project/blob/843ff7581408a02e852c0f1f7ebf176cabbc7527/clang/lib/Parse/ParseStmt.cpp#L1894-L1909
I don't quite understand why a RecoveryExpr is not created here, which caused
to the whole do statement disappears on the
AST(https://godbolt.org/z/PsPb31YKP), should we fix this?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153296/new/
https://reviews.llvm.org/D153296
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
