Izaron added a comment. > Should we maybe always treat `PotentiallyEvaluated` as `ConstantEvaluated` in > constant evaluated contexts? could that work ?
Indeed, within this patch we can prevent similars bugs to appear. I researched other places where a new context is pushed, and haven't find other bugs, but nevertheless. In my subjective opinion, the architecture of `ExprEvalContext` is pretty fragile... We could add an assert before this line <https://github.com/llvm/llvm-project/blob/0e4ecfaf5a29ca146cbcc08ed38e7b7565d4580f/clang/lib/Sema/SemaExpr.cpp#L16638>, ensuring that we don't push a (runtime) evaluated context after an immediate context. Or should we just don't push the new context in this case?... I wonder what is better, can't say right now =( In D119651#3317458 <https://reviews.llvm.org/D119651#3317458>, @cor3ntin wrote: > There seems to be quite a number of consteval related issues still - > https://reviews.llvm.org/D113859 is very similar - yet completely unrelated - > > This patch does look okay to me, in so far as it fixes this issue, in a way > that seems sensible to me. I'm just wondering if there are similar issues in > other places... BTW after looking at consteval-related issues on github <https://github.com/llvm/llvm-project/issues?q=is%3Aissue+is%3Aopen+consteval>, I've made four bite-sized patches. The issues are indeed completely unrelated to each other and do not have common source of errors. https://reviews.llvm.org/D119095 Extra constructor call - a fix in `RemoveNestedImmediateInvocation`. https://reviews.llvm.org/D119375 Trying to evaluate value-dependent ConstantExpr - a fix in `CheckForImmediateInvocation` (approved) https://reviews.llvm.org/D119646 Trying to mark declarations as "referenced" inside a ConstantExpr in default arguments - a fix in the custom def. arg. AST visitor. https://reviews.llvm.org/D119651 (This patch) a `PotentiallyEvaluated` context is created within a `ConstantEvaluated` context - remove where we do this. As far as I see, the consteval bugs rarely have common source... They mostly require ad-hoc solutions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119651/new/ https://reviews.llvm.org/D119651 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
