[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin added a comment. Thanks, landed! I have benefited a lot from your comments! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153296/new/ https://reviews.llvm.org/D153296 ___ cfe-commits mailing

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG36f67434f724: [AST] Stop evaluate constant expression if the condition expression which in… (authored by yronglin). Repository: rG LLVM Github

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM. thank you! In D153296#4480243 , @hokein wrote: > In D153296#4480141 , @yronglin >

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

hokein added a comment. In D153296#4480141 , @yronglin wrote: > In D153296#4479718 , @hokein wrote: > >> Thanks, this looks good. > > Thanks for your review! I don't know why the reversion status still `Needs >

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin added a comment. In D153296#4479718 , @hokein wrote: > Thanks, this looks good. Thanks for your review! I don't know why the reversion status still `Needs Review`, and the `libcxx ci` often fails to start. Repository: rG LLVM Github

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

hokein accepted this revision. hokein added a comment. Thanks, this looks good. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153296/new/ https://reviews.llvm.org/D153296 ___ cfe-commits mailing list

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin marked 2 inline comments as done. yronglin added a comment. Thanks for your review! Comment at: clang/lib/AST/ExprConstant.cpp:4914 static bool EvaluateDependentExpr(const Expr *E, EvalInfo ) { assert(E->isValueDependent()); aaron.ballman wrote:

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin updated this revision to Diff 537929. yronglin added a comment. Format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153296/new/ https://reviews.llvm.org/D153296 Files: clang/docs/ReleaseNotes.rst clang/lib/AST/ExprConstant.cpp

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin updated this revision to Diff 537928. yronglin marked an inline comment as done. yronglin added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153296/new/ https://reviews.llvm.org/D153296 Files:

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

aaron.ballman added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:4914 static bool EvaluateDependentExpr(const Expr *E, EvalInfo ) { assert(E->isValueDependent()); rsmith wrote: > I don't think the changes to this function are appropriate,

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

rsmith added a comment. Making the `return ESR_Failed;` unconditional looks to be the correct change here. We can't continue evaluation past that point because we don't know what would be executed next. Unconditionally returning `ESR_Failed` in that situation is what the other similar paths

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin marked an inline comment as done. yronglin added a comment. Many thanks for all of your comments, I learned a lot from the discussions, your incredible depth of knowledge have helped fundamentally shape Clang into a great compiler! It seems the common denominator is that constant

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin updated this revision to Diff 537714. yronglin added a comment. Address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153296/new/ https://reviews.llvm.org/D153296 Files: clang/docs/ReleaseNotes.rst

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

hokein added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:4921 + // value is. + if (isa(E)) +return false; aaron.ballman wrote: > hokein wrote: > > hokein wrote: > > > hokein wrote: > > > > yronglin wrote: > > > > > yronglin wrote: > > > > >

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

shafik added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:5019 if (SS->getCond()->isValueDependent()) { if (!EvaluateDependentExpr(SS->getCond(), Info)) return ESR_Failed; Please don't forget to remove this `if` and make the

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

aaron.ballman added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:4921 + // value is. + if (isa(E)) +return false; hokein wrote: > hokein wrote: > > hokein wrote: > > > yronglin wrote: > > > > yronglin wrote: > > > > > aaron.ballman wrote: >

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

hokein added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:4921 + // value is. + if (isa(E)) +return false; yronglin wrote: > yronglin wrote: > > aaron.ballman wrote: > > > yronglin wrote: > > > > hokein wrote: > > > > > The constant

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:4921 + // value is. + if (isa(E)) +return false; yronglin wrote: > aaron.ballman wrote: > > yronglin wrote: > > > hokein wrote: > > > > The constant evaluator is not aware of the

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin marked 3 inline comments as done. yronglin added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:4921 + // value is. + if (isa(E)) +return false; aaron.ballman wrote: > yronglin wrote: > > hokein wrote: > > > The constant evaluator is

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

aaron.ballman added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:4921 + // value is. + if (isa(E)) +return false; yronglin wrote: > hokein wrote: > > The constant evaluator is not aware of the "error" concept, it is only > > aware of

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin marked 3 inline comments as done. yronglin added a comment. Thanks a lot for your comments! @hokein Comment at: clang/lib/AST/ExprConstant.cpp:4921 + // value is. + if (isa(E)) +return false; hokein wrote: > The constant evaluator is not aware

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

hokein added a comment. Thanks for the fix. Sorry for being late (I was looking through the emails and found this patch). Comment at: clang/lib/AST/ExprConstant.cpp:4921 + // value is. + if (isa(E)) +return false; The constant evaluator is not aware of

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin updated this revision to Diff 536802. yronglin added a comment. Address comment. - Change `EvaluateDependentExpr`. - Add more test for do/while/for/return/ctor. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153296/new/

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin added a comment. In D153296#4468373 , @aaron.ballman wrote: > In D153296#4459769 , @yronglin > wrote: > >> Please help me, I have no better idea on this issue, do you have any better >> ideas?

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

aaron.ballman requested changes to this revision. aaron.ballman added a comment. This revision now requires changes to proceed. In D153296#4459769 , @yronglin wrote: > Please help me, I have no better idea on this issue, do you have any better > ideas?

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin added a comment. friendly ping~ 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

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin added a comment. Please help me, I have no better idea on this issue, do you have any better idea? @erichkeane @shafik Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153296/new/ https://reviews.llvm.org/D153296

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin added a comment. Seems the diagnostic message `:5:9: note: constexpr evaluation hit maximum step limit; possible infinite loop?` was redundant, also gcc dose not emit this message. https://godbolt.org/z/v55P88cdT Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin updated this revision to Diff 534606. yronglin added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153296/new/ https://reviews.llvm.org/D153296 Files: clang/docs/ReleaseNotes.rst

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

erichkeane added a comment. In D153296#4449089 , @yronglin wrote: > Can we both check `SS->getCond()->containsErrors()` ? Maybe it can avoid > bitint's effect. WDYT? @erichkeane @shafik I'm a TOUCH uncomfortable in that this only fixes the 'current'

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin added a comment. Can we both check `SS->getCond()->containsErrors()` ? Maybe it can avoid bitint's effect. WDYT? @erichkeane @shafik Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153296/new/ https://reviews.llvm.org/D153296

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

erichkeane added a comment. In D153296#4446016 , @shafik wrote: > In D153296#612 , @erichkeane > wrote: > >> So I think I'm pretty confident that the only time we would call >> `EvaluateDependentExpr` is

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin added a comment. In D153296#612 , @erichkeane wrote: > So I think I'm pretty confident that the only time we would call > `EvaluateDependentExpr` is when we are in an error condition, so I'm > convinced the fix 'as is' is incorrect. The

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

shafik added a comment. In D153296#612 , @erichkeane wrote: > So I think I'm pretty confident that the only time we would call > `EvaluateDependentExpr` is when we are in an error condition, so I'm > convinced the fix 'as is' is incorrect. The

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

erichkeane added a comment. So I think I'm pretty confident that the only time we would call `EvaluateDependentExpr` is when we are in an error condition, so I'm convinced the fix 'as is' is incorrect. The check for noteSideEffect records that we HAVE a side effect, then returns if we are OK

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

erichkeane added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:4989 if (SS->getCond()->isValueDependent()) { if (!EvaluateDependentExpr(SS->getCond(), Info)) return ESR_Failed; shafik wrote: > As far as I can tell `Value` will

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

shafik added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:4989 if (SS->getCond()->isValueDependent()) { if (!EvaluateDependentExpr(SS->getCond(), Info)) return ESR_Failed; As far as I can tell `Value` will still not be set if

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

shafik added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:4988 + if (SS->getCond()->containsErrors() || + !EvaluateDependentExpr(SS->getCond(), Info)) return ESR_Failed; yronglin wrote: > erichkeane wrote: > > It seems to me

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:4893 + // Stop evaluate if E is a RecoveryExpr. + if (isa(E)) +return false; yronglin wrote: > erichkeane wrote: > > I'd probably suggest `E->containsErrors()` instead, to cover

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin added a comment. In D153296#4442363 , @erichkeane wrote: > Just a rewording of the message, else LGTM. Thanks a lot for you're review! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153296/new/

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin updated this revision to Diff 533738. yronglin marked an inline comment as done. yronglin added a comment. Update wording. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153296/new/ https://reviews.llvm.org/D153296 Files:

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin marked an inline comment as done. yronglin added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:4893 + // Stop evaluate if E is a RecoveryExpr. + if (isa(E)) +return false; erichkeane wrote: > I'd probably suggest

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Just a rewording of the message, else LGTM. Comment at: clang/docs/ReleaseNotes.rst:523 (`#38717 _`). +- Stop

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin updated this revision to Diff 533735. yronglin marked an inline comment as done. yronglin added a comment. Add ReleaseNotes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153296/new/ https://reviews.llvm.org/D153296 Files:

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

erichkeane added a comment. Also needs a release note. Comment at: clang/lib/AST/ExprConstant.cpp:4893 + // Stop evaluate if E is a RecoveryExpr. + if (isa(E)) +return false; I'd probably suggest `E->containsErrors()` instead, to cover cases where we're

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin marked an inline comment as done. yronglin added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:4988 + if (SS->getCond()->containsErrors() || + !EvaluateDependentExpr(SS->getCond(), Info)) return ESR_Failed;

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin updated this revision to Diff 533714. yronglin added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153296/new/ https://reviews.llvm.org/D153296 Files: clang/lib/AST/ExprConstant.cpp

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

erichkeane added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:4988 + if (SS->getCond()->containsErrors() || + !EvaluateDependentExpr(SS->getCond(), Info)) return ESR_Failed; It seems to me that the 'better' solution is to

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin updated this revision to Diff 532853. yronglin added a comment. Fix test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153296/new/ https://reviews.llvm.org/D153296 Files: clang/lib/AST/ExprConstant.cpp clang/test/SemaCXX/switch.cpp

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin added a comment. Oops, compiler explorer website seems crashed, Once it's recovery, I'll append a link. :2:13: error: use of undeclared identifier 'f' 2 | switch (f) { | ^ clang++: /root/llvm-project/llvm/lib/Support/APInt.cpp:282: int

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

yronglin created this revision. Herald added a project: All. yronglin requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Signed-off-by: yronglin Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D153296 Files: