yronglin added inline comments.
================
Comment at: clang/lib/AST/ExprConstant.cpp:4921
+ // value is.
+ if (isa<RecoveryExpr>(E))
+ return false;
----------------
yronglin wrote:
> 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?
Thanks a lot for your comments! @aaron.ballman
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
