[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via llvm-commits]

cor3ntin added inline comments.


================
Comment at: clang/include/clang/Sema/Sema.h:3775
                   bool UseMemberUsingDeclRules, bool ConsiderCudaAttrs = true,
-                  bool ConsiderRequiresClauses = true);
+                  bool UseOverrideRules = false);
 
----------------
aaron.ballman wrote:
> I think we should add some comments here explaining what `UseOverrideRules` means. The old parameter was kind of mysterious as well, but this one feels even more mysterious to me.
Maybe we should document the whole function, but for that I'd need to better understand `UseMemberUsingDeclRules`

The other solution might be to have an `IsOverride` function such that both `IsOverride` and `IsOverload` function would dispatch to the same internal function.



================
Comment at: clang/lib/Sema/SemaOverload.cpp:911-915
   if (X->getNumParams() != Y->getNumParams())
     return false;
   for (unsigned I = 0; I < X->getNumParams(); ++I)
     if (!Ctx.hasSameUnqualifiedType(X->getParamDecl(I)->getType(),
                                     Y->getParamDecl(I)->getType()))
----------------
aaron.ballman wrote:
> Do we need changes here?
Yes, although I need to figure out a test


================
Comment at: clang/lib/Sema/SemaOverload.cpp:996-999
   // match than the non-reversed version.
   return FD->getNumParams() != 2 ||
          !S.Context.hasSameUnqualifiedType(FD->getParamDecl(0)->getType(),
                                            FD->getParamDecl(1)->getType()) ||
----------------
aaron.ballman wrote:
> Do we need changes here?
Yes, although I need to figure out a test


================
Comment at: clang/lib/Sema/SemaOverload.cpp:2846-2850
     // Check argument types.
     for (unsigned ArgIdx = 0, NumArgs = FromFunctionType->getNumParams();
          ArgIdx != NumArgs; ++ArgIdx) {
       QualType FromArgType = FromFunctionType->getParamType(ArgIdx);
       QualType ToArgType = ToFunctionType->getParamType(ArgIdx);
----------------
aaron.ballman wrote:
> Do we need changes here? (There may be others as well; this kind of goes back to "when do we need to care about explicit object arguments?".)
I've elected not to modify any of the Objective C code paths. I have no idea how Objective c++ inherit new features nor how deducing this would impact it.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:3170-3172
+  for (auto &&[Idx, Type] : llvm::enumerate(Old)) {
     // Reverse iterate over the parameters of `OldType` if `Reversed` is true.
+    size_t J = Reversed ? (llvm::size(New) - Idx - 1) : Idx;
----------------
aaron.ballman wrote:
> Rather than doing this dance, will `llvm::enumerate(Reversed ? llvm::reverse(Old) : Old)` work? (I've never tried composing our range-based reverse with any other range-based API.)
What would be the type of `Reversed ? llvm::reverse(Old) : Old` ? there is no common type between the two branches afaict


================
Comment at: clang/lib/Sema/SemaOverload.cpp:6250
+    ObjType = ObjType->getPointeeType();
+    Obj = UnaryOperator::Create(S.getASTContext(), Obj, UO_Deref, ObjType,
+                                VK_LValue, OK_Ordinary, SourceLocation(),
----------------
aaron.ballman wrote:
> FWIW, we don't typically treat parameters as though they were local variables unless it increases readability of the code. I don't know if this quite qualifies or not. I don't insist on a change, but may be something to keep in mind as we work through the review and update code.
Here the first line of the function would have to be

`Expr* Local = Obj;` and then Obj would not be used again. I'm keeping it, lest you insist :)


================
Comment at: clang/lib/Sema/SemaOverload.cpp:15963-15966
     // FIXME: This can't currently fail, but in principle it could.
-    return CreateBuiltinUnaryOp(UnOp->getOperatorLoc(), UO_AddrOf, SubExpr)
+    return CreateBuiltinUnaryOp(UnOp->getOperatorLoc(), UO_AddrOf,
+                                SubExpr.get())
         .get();
----------------
aaron.ballman wrote:
> 
Oh wow, I didn't spot the gnarly double conversion here. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140828