[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Aug 8 07:19:05 PDT 2023
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
More information about the cfe-commits
mailing list