hokein added inline comments.
================
Comment at: clang/lib/AST/ExprConstant.cpp:4921
+ // value is.
+ if (isa<RecoveryExpr>(E))
+ return false;
----------------
yronglin wrote:
> 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
> But for error recovery, we will *never* get a valid constant expression.
Yeah, this is true, and we don't evaluate these error-dependent expressions.
I think the question here is that when we encounter an error-dependent
expression during a constant expression evaluation, do we want to bailout the
whole evaluation immediately, or just ignore the evaluation of this
error-dependent expression and continue to proceed when possible?
We choose 2) -- this was based on the discussion with @rsmith. This will likely
give us decent error-recovery and useful followup diagnostics.
Some concrete examples,
```
int abc();
constexpr int f() {
error();
// We emit a diagnostic "Constexpr function never produces a constant
expression, because abc() cannot be used in a constant expression"
return abc();
}
```
```
// We prefer to treat the function f as a constexpr function even though it has
an error expression. We will preserve more information in the AST, e.g.
clangd's hover on the function call `f()` can give you the return value 1.
constexpr int f() {
error();
return 1;
}
```
> 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?".
Yeah, performance maybe a valid concern, but I don't have any data. These code
paths are also used for generating diagnostics. I think this is a cost we pay
for having nice error recovery.
If the performance is a real issue here, we probably need to figure out
bottlenecks before coming up solutions.
> 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.
Right.
Sorry for not being clear, I didn't mean the fix should be localized to
switch-statement condition only, we already do it for the condition for
[for](https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ExprConstant.cpp#L5452),
[while](https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ExprConstant.cpp#L5351)
etc -- whenever we encounter a value-dependent condition, we bailout
unconditionally, the switch-statement seems to be an oversight in
https://reviews.llvm.org/D113752 (sorry for not catching it earlier during the
review).
================
Comment at: clang/lib/AST/ExprConstant.cpp:4921
+ // value is.
+ if (isa<RecoveryExpr>(E))
+ return false;
----------------
hokein wrote:
> yronglin wrote:
> > 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
> > But for error recovery, we will *never* get a valid constant expression.
>
> Yeah, this is true, and we don't evaluate these error-dependent expressions.
>
> I think the question here is that when we encounter an error-dependent
> expression during a constant expression evaluation, do we want to bailout the
> whole evaluation immediately, or just ignore the evaluation of this
> error-dependent expression and continue to proceed when possible?
>
> We choose 2) -- this was based on the discussion with @rsmith. This will
> likely give us decent error-recovery and useful followup diagnostics.
>
> Some concrete examples,
>
> ```
> int abc();
> constexpr int f() {
> error();
> // We emit a diagnostic "Constexpr function never produces a constant
> expression, because abc() cannot be used in a constant expression"
> return abc();
> }
> ```
>
> ```
> // We prefer to treat the function f as a constexpr function even though it
> has an error expression. We will preserve more information in the AST, e.g.
> clangd's hover on the function call `f()` can give you the return value 1.
>
> constexpr int f() {
> error();
> return 1;
> }
> ```
>
> > 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?".
>
> Yeah, performance maybe a valid concern, but I don't have any data. These
> code paths are also used for generating diagnostics. I think this is a cost
> we pay for having nice error recovery.
> If the performance is a real issue here, we probably need to figure out
> bottlenecks before coming up solutions.
>
>
> > 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.
>
> Right.
>
> Sorry for not being clear, I didn't mean the fix should be localized to
> switch-statement condition only, we already do it for the condition for
> [for](https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ExprConstant.cpp#L5452),
>
> [while](https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ExprConstant.cpp#L5351)
> etc -- whenever we encounter a value-dependent condition, we bailout
> unconditionally, the switch-statement seems to be an oversight in
> https://reviews.llvm.org/D113752 (sorry for not catching it earlier during
> the review).
> Do you mean?
>
> if (SS->getCond()->isValueDependent()) {
> EvaluateDependentExpr(SS->getCond(), Info);
> return ESR_Failed;
> }
Yeah, this is what I meant.
================
Comment at: clang/lib/AST/ExprConstant.cpp:4921
+ // value is.
+ if (isa<RecoveryExpr>(E))
+ return false;
----------------
hokein wrote:
> hokein wrote:
> > yronglin wrote:
> > > 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
> > > But for error recovery, we will *never* get a valid constant expression.
> >
> > Yeah, this is true, and we don't evaluate these error-dependent expressions.
> >
> > I think the question here is that when we encounter an error-dependent
> > expression during a constant expression evaluation, do we want to bailout
> > the whole evaluation immediately, or just ignore the evaluation of this
> > error-dependent expression and continue to proceed when possible?
> >
> > We choose 2) -- this was based on the discussion with @rsmith. This will
> > likely give us decent error-recovery and useful followup diagnostics.
> >
> > Some concrete examples,
> >
> > ```
> > int abc();
> > constexpr int f() {
> > error();
> > // We emit a diagnostic "Constexpr function never produces a constant
> > expression, because abc() cannot be used in a constant expression"
> > return abc();
> > }
> > ```
> >
> > ```
> > // We prefer to treat the function f as a constexpr function even though it
> > has an error expression. We will preserve more information in the AST, e.g.
> > clangd's hover on the function call `f()` can give you the return value 1.
> >
> > constexpr int f() {
> > error();
> > return 1;
> > }
> > ```
> >
> > > 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?".
> >
> > Yeah, performance maybe a valid concern, but I don't have any data. These
> > code paths are also used for generating diagnostics. I think this is a cost
> > we pay for having nice error recovery.
> > If the performance is a real issue here, we probably need to figure out
> > bottlenecks before coming up solutions.
> >
> >
> > > 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.
> >
> > Right.
> >
> > Sorry for not being clear, I didn't mean the fix should be localized to
> > switch-statement condition only, we already do it for the condition for
> > [for](https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ExprConstant.cpp#L5452),
> >
> > [while](https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ExprConstant.cpp#L5351)
> > etc -- whenever we encounter a value-dependent condition, we bailout
> > unconditionally, the switch-statement seems to be an oversight in
> > https://reviews.llvm.org/D113752 (sorry for not catching it earlier during
> > the review).
> > Do you mean?
> >
> > if (SS->getCond()->isValueDependent()) {
> > EvaluateDependentExpr(SS->getCond(), Info);
> > return ESR_Failed;
> > }
>
> Yeah, this is what I meant.
>
> 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?
RecoveryExpr is created in an ad-hoc way, there are cases we might miss. It
would be nice to fix it so that clang can preserve more AST nodes for broken
code. But this is different issue, no need to address it here.
I think we run into the `if (Cond.isUsable())` branch (rather than the `else`
branch), the Cond expression is a `TypoExpr`, and we fail to resolve the
`TypoExpr`, so the whole do-while statement get discarded during the parsing.
I think we can probably fix it by passing a true `RecoverUncorrectedTypos` flag
in the `CorrectDelayedTyposInExpr` call there -- the TypoExpr will fallback to
RecoveryExpr if clang's TypoCorrection fails to resolve it.
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
