[PATCH] D111639: [Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation

[Bruno De Fraine via Phabricator via cfe-commits]

brunodf added inline comments.


================
Comment at: clang/lib/Sema/TreeTransform.h:14583
 
   if (Second && Second->getObjectKind() == OK_ObjCProperty) {
     ExprResult Result = SemaRef.CheckPlaceholderExpr(Second);
----------------
rjmccall wrote:
> rnk wrote:
> > This is also pseudo object handling code
> Hmm.  Am I wrong to be concerned about folding overload placeholders too early in these clauses?  Surely overloads can be resolved by the operator call in some cases.
> 
> I agree with Reid that it would be really nice if we could make this share the normal paths for C++ operator resolution instead of duplicating so much of them.
> Hmm.  Am I wrong to be concerned about folding overload placeholders too early in these clauses?  Surely overloads can be resolved by the operator call in some cases.

This would have to be reviewed against the paths in `Sema::BuildBinOp`? There are some cases for a RHS placeholder that does not end in calling `CheckPlaceholderExpr`:


```
 // Handle pseudo-objects in the RHS.
  if (const BuiltinType *pty = RHSExpr->getType()->getAsPlaceholderType()) {
    // An overload in the RHS can potentially be resolved by the type
    // being assigned to.
    if (Opc == BO_Assign && pty->getKind() == BuiltinType::Overload) {
      if (getLangOpts().CPlusPlus &&
          (LHSExpr->isTypeDependent() || RHSExpr->isTypeDependent() ||
           LHSExpr->getType()->isOverloadableType()))
        return BuildOverloadedBinOp(*this, S, OpLoc, Opc, LHSExpr, RHSExpr);

      return CreateBuiltinBinOp(OpLoc, Opc, LHSExpr, RHSExpr);
    }

    // Don't resolve overloads if the other type is overloadable.
    if (getLangOpts().CPlusPlus && pty->getKind() == BuiltinType::Overload &&
        LHSExpr->getType()->isOverloadableType())
      return BuildOverloadedBinOp(*this, S, OpLoc, Opc, LHSExpr, RHSExpr);

    ExprResult resolvedRHS = CheckPlaceholderExpr(RHSExpr);
    if (!resolvedRHS.isUsable()) return ExprError();
    RHSExpr = resolvedRHS.get();
  }
```

> I agree with Reid that it would be really nice if we could make this share the normal paths for C++ operator resolution instead of duplicating so much of them.

I think extracting the common placeholder logic from Build(Unary|Bin)Op and this function is beyond the scope of this patch (and probably outside my capabilities, I'm afraid). Still I hope to get things in a better shape with this patch than it was before (see PR51855). A follow up that would attempt to remove the duplication would indeed be nice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111639