[PATCH] D119651: [clang] Fix evaluation context type for consteval function calls in instantiations

Sun Feb 13 06:04:37 PST 2022

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