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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/Sema/TreeTransform.h:11940-11943
-  ExprResult Callee = getDerived().TransformExpr(E->getCallee());
-  if (Callee.isInvalid())
-    return ExprError();
-
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151553