[PATCH] D151553: [clang] Fix consteval operators in template contexts

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon added inline comments.


================
Comment at: clang/lib/Sema/TreeTransform.h:11940-11943
-  ExprResult Callee = getDerived().TransformExpr(E->getCallee());
-  if (Callee.isInvalid())
-    return ExprError();
-
----------------
rsmith wrote:
> Fznamznon wrote:
> > cor3ntin wrote:
> > > I don't understand why we would not need to transform the callee. do we do that elsewhere?
> > For example, the code above for call and subscript operators never transforms the callee.
> > This `TransFormExpr` call  seems to be a no-op for overloaded operator call case, and the result never ends up in the resulting ast.
> > 
> To me it looks like we need to transform the callee in order to transform the declarations in the `UnresolvedLookupExpr`, if the operator has such functions. For example, in:
> 
> ```
> namespace A {
>   template<typename T> T f(T t) {
>     T operator+(T, T);
>     return t + t;
>   }
> }
> namespace B {
>   struct X {};
> }
> void g(B::X x) { A::f(x); }
> ```
> 
> ... we need to transform the `UnresolvedLookupExpr` for the `t + t` to map from the `operator+` declaration in the `A::f` template to the instantiated `operator+` declaration in `A::f<B::X>`.
> 
> But I think this patch is going in the right direction. We don't *actually* want to transform the callee; all we want to do is to transform the function or list of functions contained in the callee to form an `UnresolvedSet` of candidates found during the template parse. Building and then throwing away a `DeclRefExpr` or `UnresolvedLookupExpr` denoting that function or set of functions is both a waste of time and (as demonstrated by the bug being addressed here) problematic.
> 
> Instead of calling `TransformExpr` on the callee, we should be calling `TransformOverloadExprDecls` if the callee is an `UnresolvedLookupExpr` or `TransformDecl` if it's a `DeclRefExpr`, and I think `RebuildCXXOperatorCallExpr` should be changed to take an `UnresolvedSet` and a `RequiresADL` flag, and it looks like we can then remove the `OrigCallee` parameter, if we pass in some extra source locations.
Thank you for the feedback @rsmith , I appreciate it very much.
I tried the described approach, and it fixes the original bug I was trying to resolve as well as makes the mentioned example work correctly whereas my original change broke it. Apparently, we don't have test like that in check-clang set. I'm going to polish the resulting code a bit and put up to review tomorrow.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151553/new/

https://reviews.llvm.org/D151553