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

[Erich Keane via Phabricator via cfe-commits]

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