[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Aug 7 09:21:46 PDT 2023
aaron.ballman added a subscriber: rjmccall.
aaron.ballman added a comment.
I started my review for this, but haven't completed it yet. I figured some early feedback would be better than waiting for me to complete the whole review.
================
Comment at: clang/include/clang/AST/Decl.h:1014-1015
+ /// Whether this is a C++23 explicit object parameter
+ unsigned IsExplicitObjectParameter : 1;
+
----------------
This is fine but brings this bitfield to 29 bits, so we're running out of room.
================
Comment at: clang/include/clang/AST/Decl.h:1726-1727
class ParmVarDecl : public VarDecl {
+ SourceLocation ExplicitObjectParameterIntroducerLoc;
+ friend class ASTDeclReader;
+
----------------
Might as well put this into the existing `private` section at the bottom of the class.
================
Comment at: clang/include/clang/AST/Decl.h:1817-1819
+ bool isExplicitObjectParameter() const {
+ return ParmVarDeclBits.IsExplicitObjectParameter;
+ }
----------------
Instead of stealing a bit in `ParmVarDeclBitfields`, would it make sense to instead use `return ExplicitObjectParameterIntroducerLoc.isValid();`? Then, if we disable the explicit object parameter, we can set the source location to an empty location.
================
Comment at: clang/include/clang/AST/Decl.h:1821-1822
+
+ void setIsExplicitObjectParameter(bool isExplicitObjectParam,
+ SourceLocation Loc = SourceLocation()) {
+ ParmVarDeclBits.IsExplicitObjectParameter = isExplicitObjectParam;
----------------
The default is never used anyway, also fixes a naming convention nit.
================
Comment at: clang/include/clang/AST/Decl.h:2662
+ unsigned getMinRequiredExplicitArguments() const;
+
----------------
This function could use some comments explaining how it differs from `getMinRequiredArguments()`.
================
Comment at: clang/include/clang/AST/Expr.h:1279
+ explicit DeclRefExpr(EmptyShell Empty) : Expr(DeclRefExprClass, Empty) {
+ DeclRefExprBits.CapturedByCopyInLambdaWithExplicitObjectParameter = false;
+ }
----------------
We usually only create the empty shell when we're doing something like AST deserialization, and we expect that to perform the initialization of each field manually. I think you can remove this bit?
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7249
+def err_invalid_member_use_in_method : Error<
+ "invalid use of member %0 in %select{static|explicit}1 member function">;
+
----------------
"explicit member function" is a bit confusing because of things like `explicit operator bool() const;` which is a kind of explicit member function.
There's no test coverage for this diagnostic change.
================
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>;
----------------
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?
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7280
+ "a %select{function|lambda}0 with an explicit object parameter cannot "
+ "%select{be const|be mutable|have reference qualifiers|be volatile}1">;
+def err_invalid_explicit_object_type_in_lambda: Error<
----------------
I think you're missing `restrict` here as well. Perhaps this is a case where it's better to diagnose the qualifiers generically and rely on the diagnostic's source range? e.g., `cannot have qualifiers` instead of the current %1 selection. This also works around weird with things like `void func() const &;` where it has multiple qualifiers.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7282-7283
+def err_invalid_explicit_object_type_in_lambda: Error<
+ "invalid explicit object parameter type %0 in lambda with capture"
+ "(it must be the type of the lambda or be derived from it)">;
+
----------------
Uncertain if the parenthetical bothers anyone else or not...
================
Comment at: clang/include/clang/Sema/Sema.h:5769
bool isQualifiedMemberAccess(Expr *E);
+ bool DiagnoseMissingQualifiersinAddresOfOperand(SourceLocation OpLoc,
+ Expr *Op,
----------------
A few typos to fix there.
================
Comment at: clang/include/clang/Sema/Sema.h:7847-7852
+
bool CheckDestructor(CXXDestructorDecl *Destructor);
void CheckConversionDeclarator(Declarator &D, QualType &R,
StorageClass& SC);
Decl *ActOnConversionDeclarator(CXXConversionDecl *Conversion);
+
----------------
Spurious whitespace changes.
================
Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1365-1366
Method1->isStatic() == Method2->isStatic() &&
+ Method1->isImplicitObjectMemberFunction() ==
+ Method2->isImplicitObjectMemberFunction() &&
Method1->isConst() == Method2->isConst() &&
----------------
Is this correct for structural equivalence, or are these supposed to be structurally equivalent?
```
void func(const this auto& R);
// and
void func() const &;
```
I think these aren't equivalent, but I wanted to be sure.
================
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())
----------------
Under what circumstances should existing calls to `getParamDecl()` be converted to using `getNonObjectParameter()` instead? Similar for `getNumParams()`.
================
Comment at: clang/lib/AST/DeclCXX.cpp:2412-2423
+bool CXXMethodDecl::isExplicitObjectMemberFunction() const {
+ // C++2b [dcl.fct]p6:
+ // An explicit object member function is a non-static member
+ // function with an explicit object parameter
+ return !isStatic() && getNumParams() != 0 &&
+ getParamDecl(0)->isExplicitObjectParameter();
+}
----------------
================
Comment at: clang/lib/AST/DeclCXX.cpp:2502-2503
QualType ClassTy = C.getTypeDeclType(Decl);
- return C.getQualifiedType(ClassTy, FPT->getMethodQuals());
+ auto Qualifiers = FPT->getMethodQuals();
+ return C.getQualifiedType(ClassTy, Qualifiers);
}
----------------
Can revert this NFC change (or spell out the type for `Qualifiers`).
================
Comment at: clang/lib/AST/DeclCXX.cpp:2531-2534
+ RefQualifierKind RK = FPT->getRefQualifier();
+ if (RK == RefQualifierKind::RQ_RValue)
+ return C.getRValueReferenceType(Type);
+ return C.getLValueReferenceType(Type);
----------------
What about other kinds of qualifiers like `const` and `volatile`?
================
Comment at: clang/lib/AST/DeclPrinter.cpp:868-872
+ if (ParmVarDecl *Param = dyn_cast<ParmVarDecl>(D)) {
+ if (Param->isExplicitObjectParameter()) {
+ Out << "this ";
+ }
+ }
----------------
================
Comment at: clang/lib/AST/ExprClassification.cpp:468
- if (isa<CXXMethodDecl>(D) && cast<CXXMethodDecl>(D)->isInstance())
- return Cl::CL_MemberFunction;
+ if (auto *M = dyn_cast<CXXMethodDecl>(D)) {
+ if (M->isImplicitObjectMemberFunction())
----------------
Just to double-check -- an explicit object member function is a prvalue?
================
Comment at: clang/lib/AST/ExprClassification.cpp:559-565
+ if (const auto *Method = dyn_cast<CXXMethodDecl>(Member)) {
+ if (Method->isStatic())
+ return Cl::CL_LValue;
+ if (Method->isImplicitObjectMemberFunction())
+ return Cl::CL_MemberFunction;
+ return Cl::CL_PRValue;
+ }
----------------
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:1729-1731
+ if (Method->isExplicitObjectMemberFunction()) {
+ Out << 'H';
+ }
----------------
CC @rjmccall as this relates to https://github.com/itanium-cxx-abi/cxx-abi/issues/148
================
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())
----------------
================
Comment at: clang/lib/AST/MicrosoftMangle.cpp:2749
+ // explicit object parameters are prefixed by "_V"
+ if (I == 0 && D && D->getParamDecl(0)->isExplicitObjectParameter())
+ Out << "_V";
----------------
Unsure if this adds clarity or not.
================
Comment at: clang/lib/AST/TextNodeDumper.cpp:1785-1788
+ if (const ParmVarDecl *P = dyn_cast<const ParmVarDecl>(D)) {
+ if (P->isExplicitObjectParameter())
+ OS << " this";
+ }
----------------
You should also fix up `JSONNodeDumper` at the same time.
================
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}}
----------------
================
Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:62
+ void f(this auto) {
+ this->fun(); // expected-error{{invalid use of 'this' in a function with an explicit object parameter}}
+ }
----------------
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