erichkeane added a comment. In D119651#3317619 <https://reviews.llvm.org/D119651#3317619>, @Izaron wrote:
>> 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. I like the idea of the assert, can we do it as a part of this? That way when we run into it it becomes more obvious/linked to this discussion. 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
