[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 17 10:26:31 PDT 2023
aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Sema/Sema.h:3775
bool UseMemberUsingDeclRules, bool ConsiderCudaAttrs = true,
- bool ConsiderRequiresClauses = true);
+ bool UseOverrideRules = false);
----------------
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.
================
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()))
----------------
Do we need changes here?
================
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()) ||
----------------
Do we need changes here?
================
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);
----------------
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?".)
================
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;
----------------
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.)
================
Comment at: clang/lib/Sema/SemaOverload.cpp:5583
+ // We need to have an object of class type.
+ if (const PointerType *PT = FromType->getAs<PointerType>()) {
+ FromType = PT->getPointeeType();
----------------
================
Comment at: clang/lib/Sema/SemaOverload.cpp:5673-5675
} else if (S.IsDerivedFrom(Loc, FromType, ClassType))
SecondKind = ICK_Derived_To_Base;
+ else if (!Method->isExplicitObjectMemberFunction()) {
----------------
================
Comment at: clang/lib/Sema/SemaOverload.cpp:6200
}
-
ExprResult Result = SemaRef.BuildCXXMemberCallExpr(From, Found, Conversion,
----------------
Spurious whitespace change
================
Comment at: clang/lib/Sema/SemaOverload.cpp:6224
+static QualType GetExplicitObjectType(Sema &S, Expr *MemExprE) {
+ Expr *Base = nullptr;
----------------
================
Comment at: clang/lib/Sema/SemaOverload.cpp:6229-6232
+ if (auto M = dyn_cast<UnresolvedMemberExpr>(MemExprE);
+ M && !M->isImplicitAccess())
+ Base = M->getBase();
+ else if (auto M = dyn_cast<MemberExpr>(MemExprE); M && !M->isImplicitAccess())
----------------
================
Comment at: clang/lib/Sema/SemaOverload.cpp:6234-6238
+ QualType T;
+ if (!Base)
+ T = S.getCurrentThisType();
+ else
+ T = Base->getType();
----------------
================
Comment at: clang/lib/Sema/SemaOverload.cpp:6246
+
+static Expr *GetExplicitObjectExpr(Sema &S, Expr *Obj, FunctionDecl *Fun) {
+ QualType ObjType = Obj->getType();
----------------
Can sprinkle const-correctness around here as well (same for callers, etc).
================
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(),
----------------
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.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:6254-6258
+ if (Obj->Classify(S.getASTContext()).isPRValue()) {
+ Obj = S.CreateMaterializeTemporaryExpr(
+ ObjType, Obj,
+ !Fun->getParamDecl(0)->getType()->isRValueReferenceType());
+ }
----------------
Do we have test coverage for this situation?
================
Comment at: clang/lib/Sema/SemaOverload.cpp:6351
if (Converter.match(CurToType) || ConvTemplate) {
-
if (Conversion->isExplicit()) {
----------------
Spurious whitespace change
================
Comment at: clang/lib/Sema/SemaOverload.cpp:7774-7778
+ QualType ObjectType = From->getType();
+ if (const PointerType *FromPtrType = ObjectType->getAs<PointerType>())
+ ObjectType = FromPtrType->getPointeeType();
+ CXXRecordDecl *ConversionContext =
+ cast<CXXRecordDecl>(ObjectType->castAs<RecordType>()->getDecl());
----------------
================
Comment at: clang/lib/Sema/SemaOverload.cpp:11239
// at least / at most / exactly
+ bool hasExplicitObjectParam = Fn->hasCXXExplicitFunctionObjectParameter();
+ unsigned ParamCount = FnTy->getNumParams() - (hasExplicitObjectParam ? 1 : 0);
----------------
================
Comment at: clang/lib/Sema/SemaOverload.cpp:13033
+ ExprResult Res = FixOverloadedFunctionReference(E, DAP, Found);
+ if (Res.isInvalid())
+ return false;
----------------
Below you assume that `Res.get()` returns a nonnull pointer, so you need to check for usable instead of valid. Probably worth a pass over the other uses of `isInvalid()` to see if maybe `!isUsable()` is better.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:13857
+ Expr *SubE = E;
+ CastExpr *CE = dyn_cast<CastExpr>(SubE);
+ if (CE && CE->getCastKind() == CK_NoOp)
----------------
================
Comment at: clang/lib/Sema/SemaOverload.cpp:13861
+ SubE = SubE->IgnoreParens();
+ if (CXXBindTemporaryExpr *BE = dyn_cast<CXXBindTemporaryExpr>(SubE))
+ SubE = BE->getSubExpr();
----------------
================
Comment at: clang/lib/Sema/SemaOverload.cpp:14046
Args[0] = Input;
+
CallExpr *TheCall = CXXOperatorCallExpr::Create(
----------------
Spurious whitespace change.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:14437
- if (CheckCallReturnType(FnDecl->getReturnType(), OpLoc, TheCall,
- FnDecl))
- return ExprError();
-
- ArrayRef<const Expr *> ArgsArray(Args, 2);
- const Expr *ImplicitThis = nullptr;
- // Cut off the implicit 'this'.
- if (isa<CXXMethodDecl>(FnDecl)) {
+ if (CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(FnDecl);
+ Method && Method->isImplicitObjectMemberFunction()) {
----------------
================
Comment at: clang/lib/Sema/SemaOverload.cpp:15084
+ bool HasExplicitParameter = false;
+ if (auto *M = dyn_cast<FunctionDecl>(Func);
+ M && M->hasCXXExplicitFunctionObjectParameter())
----------------
================
Comment at: clang/lib/Sema/SemaOverload.cpp:15087
+ HasExplicitParameter = true;
+ else if (auto *M = dyn_cast<FunctionTemplateDecl>(Func);
+ M &&
----------------
================
Comment at: clang/lib/Sema/SemaOverload.cpp:15256-15258
if ((isa<CXXConstructorDecl>(CurContext) ||
isa<CXXDestructorDecl>(CurContext)) &&
+ TheCall->getDirectCallee()->isPure()) {
----------------
================
Comment at: clang/lib/Sema/SemaOverload.cpp:15275
if (CXXDestructorDecl *DD =
+ dyn_cast<CXXDestructorDecl>(TheCall->getDirectCallee())) {
----------------
================
Comment at: clang/lib/Sema/SemaOverload.cpp:15512
+ if (Method->isExplicitObjectMemberFunction()) {
+ // FIXME: we should do that during the definition of the lambda when we can
+ DiagnoseInvalidExplicitObjectParameterInLambda(Method);
----------------
================
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();
----------------
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