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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.


================
Comment at: clang/include/clang/AST/Expr.h:1279
+  explicit DeclRefExpr(EmptyShell Empty) : Expr(DeclRefExprClass, Empty) {
+    DeclRefExprBits.CapturedByCopyInLambdaWithExplicitObjectParameter = false;
+  }
----------------
aaron.ballman wrote:
> We usually only create the empty shell when we're doing something like AST deserialization, and we expect that to perform the initialization of each field manually. I think you can remove this bit?
I moved it to the serialization code, but this fields is never serialized (It stop being useful once the dependency is computed). 
Maybe DeclRefExprBits is not the best place but at the same time this way it doesn't take too much space


================
Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1365-1366
       Method1->isStatic() == Method2->isStatic() &&
+      Method1->isImplicitObjectMemberFunction() ==
+          Method2->isImplicitObjectMemberFunction() &&
       Method1->isConst() == Method2->isConst() &&
----------------
aaron.ballman wrote:
> Is this correct for structural equivalence, or are these supposed to be structurally equivalent?
> ```
> void func(const this auto& R);
> // and
> void func() const &;
> ```
> I think these aren't equivalent, but I wanted to be sure.
Yup, they have completely different type


================
Comment at: clang/lib/AST/DeclCXX.cpp:841
       const auto *ParamTy =
-          Method->getParamDecl(0)->getType()->getAs<ReferenceType>();
+          Method->getNonObjectParameter(0)->getType()->getAs<ReferenceType>();
       if (!ParamTy || ParamTy->getPointeeType().isConstQualified())
----------------
aaron.ballman wrote:
> Under what circumstances should existing calls to `getParamDecl()` be converted to using `getNonObjectParameter()` instead? Similar for `getNumParams()`.
everytime you want to ignore (or handle differently) the explicit object parameter. I'll do another survey at some point, to be sure i didn't miss a spot


================
Comment at: clang/lib/AST/DeclCXX.cpp:2531-2534
+  RefQualifierKind RK = FPT->getRefQualifier();
+  if (RK == RefQualifierKind::RQ_RValue)
+    return C.getRValueReferenceType(Type);
+  return C.getLValueReferenceType(Type);
----------------
aaron.ballman wrote:
> What about other kinds of qualifiers like `const` and `volatile`?
`::getThisObjectType` does that for us


================
Comment at: clang/lib/AST/ExprClassification.cpp:468
 
-  if (isa<CXXMethodDecl>(D) && cast<CXXMethodDecl>(D)->isInstance())
-    return Cl::CL_MemberFunction;
+  if (auto *M = dyn_cast<CXXMethodDecl>(D)) {
+    if (M->isImplicitObjectMemberFunction())
----------------
aaron.ballman wrote:
> Just to double-check -- an explicit object member function is a prvalue?
This is my reading of https://eel.is/c++draft/expr.prim#id.qual-5 



================
Comment at: clang/lib/AST/ExprClassification.cpp:559-565
+  if (const auto *Method = dyn_cast<CXXMethodDecl>(Member)) {
+    if (Method->isStatic())
+      return Cl::CL_LValue;
+    if (Method->isImplicitObjectMemberFunction())
+      return Cl::CL_MemberFunction;
+    return Cl::CL_PRValue;
+  }
----------------
aaron.ballman wrote:
> 
Can you confirm? I know it's redundant, but it matches the comment and i don't think removing it is an improvement


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140828