[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)
Erich Keane via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 25 07:37:54 PDT 2023
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
More information about the cfe-commits
mailing list