[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 27 11:26:14 PDT 2023
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
More information about the cfe-commits
mailing list