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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Mostly just nits (plus my hatred for C style casts :) ), I didn't see any issues with the approach.



================
Comment at: clang/include/clang/AST/ASTLambda.h:45
+  return isLambdaCallOperator(DC) &&
+         !cast<CXXMethodDecl>(DC)->getType().isNull() &&
+         !cast<CXXMethodDecl>(DC)->isExplicitObjectMemberFunction();
----------------
Under what cases does this end up being null?  It seems like this condition shouldn't be necessary, and it doesn't seem like we should have a case where we create a method/function without a type set?


================
Comment at: clang/lib/AST/Decl.cpp:3624
+unsigned FunctionDecl::getNumNonObjectParams() const {
+  return getNumParams() - (unsigned)hasCXXExplicitFunctionObjectParameter();
+}
----------------



================
Comment at: clang/lib/AST/Decl.cpp:3629
+  return getMinRequiredArguments() -
+         (hasCXXExplicitFunctionObjectParameter() ? 1 : 0);
+}
----------------



================
Comment at: clang/lib/CodeGen/CGExprCXX.cpp:72
+      if (const auto *M = dyn_cast<CXXMethodDecl>(Op->getCalleeDecl()))
+        ArgsToSkip = (unsigned)!M->isExplicitObjectMemberFunction();
+    }
----------------



================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:14942
+  ExprBuilder &ObjectParameter =
+      ExplicitObject ? (ExprBuilder &)*ExplicitObject : (ExprBuilder &)*This;
 
----------------
These casts should likely not be C-style/clobber casts.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:14965
+    CastBuilder To(
+        ExplicitObject ? (ExprBuilder &)*ExplicitObject
+                       : (ExprBuilder &)*DerefThis,
----------------
same here


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:15050
   if (!Invalid) {
     // Add a "return *this;"
+    Expr *ThisExpr = (ExplicitObject  ? (ExprBuilder &)*ExplicitObject
----------------
here and elsewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140828