aaron.ballman added reviewers: rjmccall, efriedma. aaron.ballman added a comment.
I think we're getting pretty close! My goal is to get this landed ASAP; I do not think it needs to be hidden behind a feature flag (-std=c++2b is sufficient), and it's good that we're not defining the feature test macro yet. There were a few outstanding questions still that would be good to get answers for. Having some codegen code owner eyes on this would be appreciated, so adding them as reviewers. ================ Comment at: clang/include/clang/AST/ASTLambda.h:45 + return isLambdaCallOperator(DC) && + !cast<CXXMethodDecl>(DC)->getType().isNull() && + !cast<CXXMethodDecl>(DC)->isExplicitObjectMemberFunction(); ---------------- cor3ntin wrote: > erichkeane wrote: > > 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? > If you have invalid captures for example, you can end up with no type. (but > we do need to create a method because you need to have a context to which > attach the captures to) This does feel rather unclean, though it may be fine for now. Instead of a null type, I would expect the declaration to be marked as invalid (and perhaps for callers to avoid calling `isLambdaCallWithImplicitObjectParameter()` on an invalid declaration so that we can turn the predicate into an assertion). Let's add a FIXME for now? ================ Comment at: clang/include/clang/AST/Decl.h:1724-1725 /// Represents a parameter to a function. class ParmVarDecl : public VarDecl { + public: ---------------- spurious whitespace ================ Comment at: clang/include/clang/AST/DeclCXX.h:2063-2064 bool isInstance() const { return !isStatic(); } + bool isExplicitObjectMemberFunction() const; + bool isImplicitObjectMemberFunction() const; + const ParmVarDecl *getNonObjectParameter(unsigned I) const { ---------------- tbaeder wrote: > Can you add some documentation to these? Still needs some docs explaining the difference for folks. ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7299-7302 +def err_explicit_object_default_arg: Error< + "the explicit object parameter cannot have a default argument">; +def err_explicit_object_parameter_pack: Error< + "the explicit object parameter cannot be a function parameter pack">; ---------------- ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9479-9480 def err_defaulted_comparison_num_args : Error< "%select{non-member|member}0 %sub{select_defaulted_comparison_kind}1" - " comparison operator must have %select{2|1}0 parameters">; + " must have %select{2|1}0 parameters">; def err_defaulted_comparison_param : Error< ---------------- I don't see any tests modified as a result of this change (and the change seems orthogonal in a similar way as the above changes). ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7260-7265 +def ext_deducing_this : ExtWarn< + "explicit object parameters are a C++2b extension">, + InGroup<CXX23>; +def warn_cxx20_ext_deducing_this : Warning< + "explicit object parameters are incompatible with C++ standards before C++2b">, + DefaultIgnore, InGroup<CXXPre23Compat>; ---------------- cor3ntin wrote: > aaron.ballman wrote: > > Missing test coverage for both of these. > > > > What is the rationale for exposing this as an extension in earlier language > > modes instead of leaving this a C++26 and later feature? > Can you clarify, what do you think is missing test coverage? `explicit object parameters are incompatible with C++ standards before C++2b` is missing test coverage. ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9378-9381 + "an explicitly-defaulted %sub{select_special_member_kind}0 cannot " "have default arguments">; def err_defaulted_special_member_variadic : Error< + "an explicitly-defaulted %sub{select_special_member_kind}0 cannot " ---------------- aaron.ballman wrote: > These changes seem like they're orthogonal to the patch. Should we split them > off into their own NFC commit so we can get them out of here? These changes can still be split off into their own NFC commit. ================ Comment at: clang/include/clang/Sema/Sema.h:3775 bool UseMemberUsingDeclRules, bool ConsiderCudaAttrs = true, - bool ConsiderRequiresClauses = true); + bool UseOverrideRules = false); ---------------- cor3ntin wrote: > aaron.ballman wrote: > > 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. > Maybe we should document the whole function, but for that I'd need to better > understand `UseMemberUsingDeclRules` > > The other solution might be to have an `IsOverride` function such that both > `IsOverride` and `IsOverload` function would dispatch to the same internal > function. > > The other solution might be to have an IsOverride function such that both > IsOverride and IsOverload function would dispatch to the same internal > function. I think that's a clean solution. ================ Comment at: clang/lib/AST/ComputeDependence.cpp:502-504 + if (E->isCapturedByCopyInLambdaWithExplicitObjectParameter()) { + Deps |= ExprDependence::Type; + } ---------------- ================ Comment at: clang/lib/AST/DeclCXX.cpp:2527 + ASTContext &C = getParentASTContext(); + const FunctionProtoType *FPT = getType()->castAs<FunctionProtoType>(); + QualType Type = ::getThisObjectType(C, FPT, getParent()); ---------------- ================ 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()) ---------------- cor3ntin wrote: > aaron.ballman wrote: > > cor3ntin wrote: > > > cor3ntin wrote: > > > > 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 > > > I found a bug https://github.com/cplusplus/CWG/issues/390 ! > > > (`AreSpecialMemberFunctionsSameKind`) - So far nothing else but I'll do > > > several passes > > This adds an unfortunate amount of complexity to the compiler because now > > we have to explicitly remember to think about this weird scenario, but I'm > > not seeing much of a way around it. I suspect this will be a bit of a bug > > factory until we're used to thinking explicitly about this. > > > > Be sure to double-check things like attributes that take arguments which > > represent an index into a function parameter list (like the `format` > > attribute), as those have to do a special dance to handle the implicit > > `this` parameter. I'm guessing the static analyzer and clang-tidy will both > > also run into this in some places as well. > https://cplusplus.github.io/CWG/issues/2787.html > Do we want to implement that resolution now? > https://cplusplus.github.io/CWG/issues/2787.html > Do we want to implement that resolution now? I think it's fine to do that resolution in a follow-up closely after landing this, unless you want to do it now because it's trivial enough. I don't imagine CWG is going to come back with a drastically different resolution, do you? CC @shafik for opinions on reading CWG tea leaves. ================ Comment at: clang/lib/AST/ItaniumMangle.cpp:1729-1731 + if (Method->isExplicitObjectMemberFunction()) { + Out << 'H'; + } ---------------- aaron.ballman wrote: > CC @rjmccall as this relates to > https://github.com/itanium-cxx-abi/cxx-abi/issues/148 Based on the discussion in that thread, I think this direction is reasonable. Ping @rjmccall for any last-minute concerns before we make this part of the ABI. ================ Comment at: clang/lib/CodeGen/CGExpr.cpp:4254 + llvm::Value *ThisValue) { + bool HasExplicitObjectParameter = false; + if (const CXXMethodDecl *MD = ---------------- aaron.ballman wrote: > 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. Still hoping @efriedma or @rjmccall can comment on the codegen changes. ================ Comment at: clang/lib/Parse/ParseDecl.cpp:5756 const ParsedTemplateInfo *TemplateInfo) { - TentativeParsingAction TPA(*this); - + RevertingTentativeParsingAction TPA(*this); // Parse the C++ scope specifier. ---------------- aaron.ballman wrote: > This is a good NFC cleanup; feel free to land this change separately if you'd > like to get it out of this review. This can still be split off to make the review shorter for folks. ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11342 + << D.getSourceRange() << /*static=*/0 << IsLambda; + return; + } ---------------- I think this early return should also be pulled. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:20653 + Sema &SemaRef, ValueDecl *D, Expr *E) { + DeclRefExpr *ID = dyn_cast<DeclRefExpr>(E); + if (!ID || ID->isTypeDependent()) ---------------- ================ Comment at: clang/lib/Sema/SemaOverload.cpp:1299-1300 + + // We do not allow overloading based off of '__restrict'. + Q.removeRestrict(); + ---------------- cor3ntin wrote: > aaron.ballman wrote: > > Just restrict? So `_Nonnull` is fine? > Good question. This was pre-existing code though. I'll think about it more Any new thoughts on this? ================ 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); ---------------- cor3ntin wrote: > aaron.ballman wrote: > > 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?".) > I've elected not to modify any of the Objective C code paths. I have no idea > how Objective c++ inherit new features nor how deducing this would impact it. Objective-C++ is a pure extension over the top of C++, so C++ functionality should behave in the expected way in ObjC++. But because this is with ObjC pointers (which are kind of a special thing), it's not clear to me either. I say let's punt on it; but final word comes from @rjmccall ================ 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; ---------------- cor3ntin wrote: > aaron.ballman wrote: > > 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.) > What would be the type of `Reversed ? llvm::reverse(Old) : Old` ? there is no > common type between the two branches afaict Oh derp. :-) ================ 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(), ---------------- cor3ntin wrote: > aaron.ballman wrote: > > 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. > Here the first line of the function would have to be > > `Expr* Local = Obj;` and then Obj would not be used again. I'm keeping it, > lest you insist :) I don't insist. ================ 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()); + } ---------------- aaron.ballman wrote: > Do we have test coverage for this situation? Still wondering about test coverage for this bit. ================ Comment at: clang/test/CodeGenCXX/cxx2b-deducing-this.cpp:4 + +struct TrivialStruct { + void explicit_object_function(this TrivialStruct) {} ---------------- cor3ntin wrote: > cor3ntin wrote: > > aaron.ballman wrote: > > > I'd like to see more codegen tests in general -- for example, a test that > > > demonstrates we properly handle code like: > > > ``` > > > struct B { > > > virtual void f(); > > > }; > > > > > > struct T {}; > > > struct D3 : B { > > > void f(this T&); // Okay, not an override > > > }; > > > > > > void func() { > > > T t; > > > t.f(); // Verify this calls D3::f() and not B::f() > > > } > > > ``` > > > but also tests that show that we do the correct thing for calling > > > conventions (do explicit object parameter functions act as `__fastcall` > > > functions?), explicit object parameters in lambdas, call through a > > > pointer to member function, and so on. > > > > > > Another test that could be interesting is how chained calls look > > > (roughly): > > > ``` > > > struct S { > > > void foo(this const S&); > > > }; > > > > > > struct T { > > > S bar(this const &T); > > > }; > > > > > > void func() { > > > T t; > > > t.bar().foo(); > > > } > > > ``` > > That first example is ill-formed (it is an override, not allowed) > > > > I will need help for codegen tests. > > For `__thiscall`, are we even doing the same thing? > > https://compiler-explorer.com/z/KTea6W36T > NVM, just different optimization levels > That first example is ill-formed (it is an override, not allowed) Oh you're right, I missed a very important "not" in this chain of logic: https://eel.is/c++draft/class#virtual-2.sentence-1 https://eel.is/c++draft/basic.scope.scope#4.3 https://eel.is/c++draft/basic.scope.scope#4.4 Coupled with the direction in CWG2553 and CWG2554. ================ 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}} +}; ---------------- cor3ntin wrote: > aaron.ballman wrote: > > If possible, it would be nicer if we only issued one warning as the root > > cause is the same for both diagnostics. > Should we simply remove the newly added diagnostic then? I think the new diagnostic is helpful; I'm more wondering if we can emit just the new diagnostic and skip `destructor cannot have any parameters` if we already said it can't have an explicit object parameter? ================ Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:192 + f(); // expected-error{{call to non-static member function without an object argument}} + f(Over_Call_Func_Example{}); // expected-error{{call to non-static member function without an object argument}} + Over_Call_Func_Example{}.f(); // ok ---------------- cor3ntin wrote: > aaron.ballman wrote: > > Errr, this diagnostic seems likely to be hard for users to act on: this > > non-static member function has an object argument! And it's even the right > > type! > > > > If the model for the feature is "these are just static functions with funky > > lookup rules", I kind of wonder if this should be accepted instead of > > rejected. But if it needs to be rejected, I think we should have a better > > diagnostic, perhaps along the lines of "a call to an explicit object member > > function cannot specify the explicit object argument" with a fix-it to > > remove that argument. WDYT? > UGH, phab is confused, I'm no longer sure which diag you are concerned > about... ``` static void h() { f(); // expected-error{{call to non-static member function without an object argument}} f(Over_Call_Func_Example{}); // expected-error{{call to non-static member function without an object argument}} Over_Call_Func_Example{}.f(); // ok } ``` from around line 214 in the latest patch; the second error seems hard to understand because there is an object argument being passed, it's just not the form allowed. 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