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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
