aaron.ballman added a subscriber: efriedma. aaron.ballman added a comment. Round two of comments; I picked up from where I left off earlier and haven't checked your changes or responses yet. Precommit CI seems to have found a relevant failure from the patch.
================ Comment at: clang/lib/AST/ExprConstant.cpp:7768-7770 + bool HasThis = + isa<CXXMethodDecl>(FD) && + cast<CXXMethodDecl>(FD)->isImplicitObjectMemberFunction(); ---------------- ================ Comment at: clang/lib/AST/JSONNodeDumper.cpp:846 JOS.attribute("type", createQualType(VD->getType())); + if (const auto *P = dyn_cast<const ParmVarDecl>(VD)) + attributeOnlyIfTrue("explicitObjectParameter", ---------------- ================ Comment at: clang/lib/AST/MicrosoftMangle.cpp:2748 for (unsigned I = 0, E = Proto->getNumParams(); I != E; ++I) { + // explicit object parameters are prefixed by "_V" + if (I == 0 && D && D->getParamDecl(0)->isExplicitObjectParameter()) ---------------- aaron.ballman wrote: > Still missing the full stop at the end of the sentence. ================ Comment at: clang/lib/AST/TextNodeDumper.cpp:1785 dumpName(D); + if (const auto *P = dyn_cast<const ParmVarDecl>(D); + P && P->isExplicitObjectParameter()) ---------------- Oops, I missed this suggestion previously. ================ Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2129-2140 !D->hasAttr<CUDADeviceAttr>()) { const CXXMethodDecl *MD; // Variable pointer template parameters have a value that is the address // of the variable. if (const auto *VD = dyn_cast<VarDecl>(D)) V = CGM.GetAddrOfGlobalVar(VD); // Member function pointers have special support for building them, ---------------- Might as well clean this up since we're in the area. ================ Comment at: clang/lib/CodeGen/CGExpr.cpp:4275-4276 + bool HasExplicitObjectParameter = false; + if (const CXXMethodDecl *MD = + dyn_cast_if_present<CXXMethodDecl>(CurCodeDecl)) { + HasExplicitObjectParameter = MD->isExplicitObjectMemberFunction(); ---------------- ================ Comment at: clang/lib/CodeGen/CGExpr.cpp:4283 + if (HasExplicitObjectParameter) { + const VarDecl *D = cast<CXXMethodDecl>(CurCodeDecl)->getParamDecl(0); + auto It = LocalDeclMap.find(D); ---------------- Above we did `dyn_cast_if_present<CXXMethodDecl>(CurCodeDecl)` and here we're doing a straight-up `cast<>`. Which is incorrect? ================ Comment at: clang/lib/CodeGen/CGExpr.cpp:5015-5017 + // a CXXOperatorCallExpr is created even for + // explicit object methods, but these should be treated + // like static function call ---------------- ================ Comment at: clang/lib/CodeGen/CGExpr.cpp:5019-5020 if (const auto *CE = dyn_cast<CXXOperatorCallExpr>(E)) if (const CXXMethodDecl *MD = - dyn_cast_or_null<CXXMethodDecl>(CE->getCalleeDecl())) + dyn_cast_or_null<CXXMethodDecl>(CE->getCalleeDecl()); + MD && MD->isImplicitObjectMemberFunction()) ---------------- ================ Comment at: clang/lib/CodeGen/CGExpr.cpp:4254 + llvm::Value *ThisValue) { + bool HasExplicitObjectParameter = false; + if (const CXXMethodDecl *MD = ---------------- cor3ntin wrote: > erichkeane wrote: > > If there is a CodeGen expert in the house, I'd really hope they go through > > this :) > Oh, me too! > I need to properly write tests for it too. And you know how terrible I am at > code gen tests... CC @efriedma @rjmccall for codegen opinions. ================ Comment at: clang/lib/CodeGen/CGExprCXX.cpp:69-73 + unsigned ArgsToSkip = 0; + if (auto *Op = llvm::dyn_cast<CXXOperatorCallExpr>(CE)) { + if (const CXXMethodDecl *M = dyn_cast<CXXMethodDecl>(Op->getCalleeDecl())) + ArgsToSkip = M->isExplicitObjectMemberFunction() ? 0 : 1; + } ---------------- ================ Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:1170 - if (isa_and_nonnull<CXXMethodDecl>(D) && - cast<CXXMethodDecl>(D)->isInstance()) { - CGM.getCXXABI().EmitInstanceFunctionProlog(*this); - const CXXMethodDecl *MD = cast<CXXMethodDecl>(D); - if (MD->getParent()->isLambda() && - MD->getOverloadedOperator() == OO_Call) { + if (const CXXMethodDecl *MD = dyn_cast_if_present<CXXMethodDecl>(D); + MD && !MD->isStatic()) { ---------------- ================ Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2240-2241 // need this sort of type metadata. - return !MD->isStatic() && !MD->isVirtual() && !isa<CXXConstructorDecl>(MD) && - !isa<CXXDestructorDecl>(MD); + return MD->isImplicitObjectMemberFunction() && !MD->isVirtual() && + !isa<CXXConstructorDecl>(MD) && !isa<CXXDestructorDecl>(MD); } ---------------- ================ Comment at: clang/lib/Parse/ParseDecl.cpp:5756 const ParsedTemplateInfo *TemplateInfo) { - TentativeParsingAction TPA(*this); - + RevertingTentativeParsingAction TPA(*this); // Parse the C++ scope specifier. ---------------- This is a good NFC cleanup; feel free to land this change separately if you'd like to get it out of this review. ================ Comment at: clang/lib/Parse/ParseDecl.cpp:7342-7347 + // Parse a C++23 Explicit Object Parameter + SourceLocation ThisLoc; + if (getLangOpts().CPlusPlus && Tok.is(tok::kw_this)) { + ThisLoc = ConsumeToken(); + } + ---------------- You might want to leave a comment explaining why you're parsing `this` in early C++ language modes. ================ Comment at: clang/lib/Parse/ParseTentative.cpp:1563 + case tok::kw_this: { + if (getLangOpts().CPlusPlus) { + RevertingTentativeParsingAction PA(*this); ---------------- Should this be checking for CPlusPlus23 instead? ================ Comment at: clang/lib/Parse/ParseTentative.cpp:1569 + } + [[fallthrough]]; + } ---------------- Why do we want this to fall through to the annotated template id case? ================ Comment at: clang/lib/Sema/SemaCoroutine.cpp:483-487 if (auto *MD = dyn_cast_or_null<CXXMethodDecl>(FD)) - return MD->isInstance() && MD->getThisType()->isDependentType(); + return MD->isImplicitObjectMemberFunction() && + MD->getThisType()->isDependentType(); else return false; ---------------- ================ Comment at: clang/lib/Sema/SemaDecl.cpp:14730 + "explicit parameter in non-cplusplus mode"); + if(!S.getLangOpts().CPlusPlus23) + S.Diag(ExplicitThisLoc, diag::err_cxx20_deducing_this) ---------------- ================ Comment at: clang/lib/Sema/SemaDecl.cpp:14735 + // C++2b [dcl.fct/7] An explicit object parameter shall not be a function + // parameter pack + if (P->isParameterPack()) { ---------------- ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:7658 // A defaulted special member cannot have cv-qualifiers. - if (Type->getMethodQuals().hasConst() || Type->getMethodQuals().hasVolatile()) { + if (ThisType.isConstQualified() || ThisType.isVolatileQualified()) { if (DeleteOnTypeMismatch) ---------------- Ruh roh, existing bug: https://godbolt.org/z/oefa3hPcd -- should probably be looking for any qualifiers, not just cv qualifiers. I filed https://github.com/llvm/llvm-project/issues/64530 to track this. ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:7671 + QualType ArgType = + ExpectedParams && Type + ? Type->getParamType(MD->isExplicitObjectMemberFunction() ? 1 : 0) ---------------- `Type` cannot be null (it's produced from a call to `cast<>`). ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8419-8420 ExprResult LHS; - if (isa<CXXMethodDecl>(FD)) { + if (auto *MD = dyn_cast<CXXMethodDecl>(FD); + MD && MD->isImplicitObjectMemberFunction()) { // LHS is '*this'. ---------------- I'm seeing this pattern enough that I wonder if it makes sense to add a member function to `FunctionDecl` just to do this dance for us? It'd be a bit weird because `FunctionDecl` is never a member function, but we have some helper functionality already related to member functions in the class (`isOutOfLine()` and `getInstantiatedFromMemberFunction()` come to mind). ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8765-8766 - if (FD->getNumParams() != (IsMethod ? 1 : 2)) { + if (FD->getNumParams() - + (FD->hasCXXExplicitFunctionObjectParameter() ? 1 : 0) != + (IsMethod ? 1 : 2)) { ---------------- ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11047 + // [C++2b] + // A conversion function shall have no non-object parameters + if (NumParam == 1) { ---------------- ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11050 + DeclaratorChunk::FunctionTypeInfo &FTI = D.getFunctionTypeInfo(); + if (ParmVarDecl *First = dyn_cast_or_null<ParmVarDecl>(FTI.Params[0].Param); + First && First->isExplicitObjectParameter()) ---------------- ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11264-11278 + return; + } + + if (D.getDeclSpec().isVirtualSpecified()) { + Diag(ExplicitObjectParam->getBeginLoc(), + diag::err_explicit_object_parameter_nonmember) + << D.getSourceRange() << 1 << IsLambda; ---------------- Should we pull the early returns from these? Or should we add an early return to the default argument case? It's a bit odd that it's inconsistent; my intuition is we want two diagnostics for `virtual void func(this int i = 12);` Also, can you add comment in front of the magic numbers, as in `/*foo=*/1` (same elsewhere in this function)? ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11291 + + assert(DC && "Expected a non-null declaration context"); + ---------------- The assert doesn't do much given the `if` statement directly above it; WDYT? ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11293-11301 + if (FTI.hasMethodTypeQualifiers()) { + if (FTI.getConstQualifierLoc().isValid()) + Diag(FTI.getConstQualifierLoc(), + diag::err_explicit_object_parameter_qualifiers) + << 0 << 0 << D.getSourceRange(); + if (FTI.getVolatileQualifierLoc().isValid()) + Diag(FTI.getVolatileQualifierLoc(), ---------------- Should look for any kind of qualifiers, not just `const` and `volatile`. ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11308 + + // There should be a CWG issue for that + if (Name.getNameKind() == DeclarationName::CXXConstructorName || ---------------- Do you know the core issue number so we can reference it here? ================ Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:29-30 + S(this auto); // expected-error {{an explicit object parameter cannot appear in a constructor}} + ~S(this S) {} // expected-error {{an explicit object parameter cannot appear in a destructor}} \ + // expected-error {{destructor cannot have any parameters}} +}; ---------------- If possible, it would be nicer if we only issued one warning as the root cause is the same for both diagnostics. ================ Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:42 + int f(this B&, int); // expected-warning {{hides overloaded virtual function}} + int f(this B&); // expected-error {{an explicit object parameter cannot appear in a virtual function}} + int g(this B&); // expected-warning {{hides overloaded virtual function}} ---------------- According to the paper, this is accepted but `B::f()` does not override `A::f()`. https://eel.is/c++draft/dcl.fct#6.sentence-3 `B::f()` is not declared as virtual, only `A::f()` is. When this gets corrected, please add some codegen tests to demonstrate that a call to `f()` does not dispatch to `B::f()` unless the static type of the object is `B`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140828/new/ https://reviews.llvm.org/D140828 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits