[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 10 07:49:46 PDT 2023
aaron.ballman added inline comments.
================
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<
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > cor3ntin wrote:
> > > aaron.ballman wrote:
> > > > 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.
> > > Forming a source range to the qualifiers may be challenging though.
> > >
> > > In what case would `restrict` come into play?
> > >
> > > ```
> > > struct test {
> > > void f() restrict;
> > > };
> > > ```
> > > does not seem valid, I'm assuming it is in some language mode?
> > Ah, it's spelled `__restrict` in C++ mode, but we also support other qualifiers like `_Nonnull` as well. I left some examples in other comments that should demonstrate this.
> Maybe we need a way to compute the range of all qualifiers. I'll look into that - I'm not sure the information exists atm.
> SourceLocation objects are ordered, right?
Correct, source locations are ordered; you can use `SourceManager::isBeforeInTranslationUnit()` to compare orderings within the same TU.
================
Comment at: clang/lib/Parse/ParseTentative.cpp:1563
+ case tok::kw_this: {
+ if (getLangOpts().CPlusPlus) {
+ RevertingTentativeParsingAction PA(*this);
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > Should this be checking for CPlusPlus23 instead?
> Nope, again we don't want to produce terrible error messages!
Thanks for confirming; I thought that was the case. I think it's fine because it won't impact any valid code (I was momentarily worried this would change parsing behavior in older language modes, but it won't).
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:14924
- // Builds the "this" pointer.
- ThisBuilder This;
+ // Builds the function object parameter
+ std::optional<ThisBuilder> This;
----------------
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:15299
- // Builds the "this" pointer.
- ThisBuilder This;
+ // Builds the function object parameter
+ std::optional<ThisBuilder> This;
----------------
<Not your problem>There is so much code duplication in here... it'd be nice to unify the copy and move assign functionality as much as we can.</Not your problem>
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:15436-15437
if (!Invalid) {
// Add a "return *this;"
- ExprResult ThisObj =
- CreateBuiltinUnaryOp(Loc, UO_Deref, This.build(*this, Loc));
+ // Add a "return *this;"
+ Expr *ThisExpr = (ExplicitObject ? (ExprBuilder &)*ExplicitObject
----------------
It's important to know what's happening here, but not so important you need to repeat it. ;-)
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:18171
+bool Sema::CheckOverridingExplicitObjectMembers(CXXMethodDecl *New,
+ const CXXMethodDecl *Old) {
----------------
This function name feels off to me; would it make more sense as `DiagnoseExplicitObjectOverride()` or something along those lines? It's not really doing a general check, just checking one specific property.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:18174
+ // CWG2553
+ // A virtual function shall not be an explicit object member function
+ if (!New->isExplicitObjectMemberFunction())
----------------
================
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'.
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > 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).
> It might be weird because a FunctionDecl might be neither explicit, not implicit, nor static... knowing it's not explicit is only useful in the places we look at it in this PR basically.
> Or we'd need some `enum { NonMember, Static, Implicit, Explicit }` to cover our bases, but I'm not sure it would be worth it.
Okay, let's hold off on changes for now then.
================
Comment at: clang/lib/Sema/SemaExceptionSpec.cpp:899
+ const PartialDiagnostic &DiagID, const PartialDiagnostic &NoteID,
+ const FunctionProtoType *Target, unsigned SkipTargetFirstParameter,
+ SourceLocation TargetLoc, const FunctionProtoType *Source,
----------------
Should the `Skip` parameters be `bool` so that the interface to the function is a bit more readable? I assume you don't expect anyone to call this with `42` or something.
================
Comment at: clang/lib/Sema/SemaExceptionSpec.cpp:916-917
for (unsigned i = 0, E = Target->getNumParams(); i != E; ++i) {
+ // Target->getExtParameterInfo(0).
+
auto ParamDiag = DiagID;
----------------
An odd comment to add, is something missing here or should this just be removed?
================
Comment at: clang/lib/Sema/SemaExpr.cpp:14887
+bool Sema::DiagnoseMissingQualifiersInAddressOfOperand(
+ SourceLocation OpLoc, Expr *Op, const CXXMethodDecl *MD) {
----------------
aaron.ballman wrote:
> Can we get away with it, or does this require viral changes?
`Diagnose*` functions typically return true when an error is emitted and false otherwise, so I'd recommend switching the logic around somewhat. Also, this function name doesn't really match what the function does -- we're checking for things unrelated to missing qualifiers, like use of parens and taking the address of a destructor. We should probably generalize the name at the same time (I'm not even certain how one has missing qualifiers in an address of operand).
================
Comment at: clang/lib/Sema/SemaExpr.cpp:14887-14889
+bool Sema::DiagnoseMissingQualifiersInAddressOfOperand(
+ SourceLocation OpLoc, Expr *Op, const CXXMethodDecl *MD) {
+ auto *DRE = cast<DeclRefExpr>(Op->IgnoreParens());
----------------
Can we get away with it, or does this require viral changes?
================
Comment at: clang/lib/Sema/SemaExpr.cpp:14891-14894
+ if (Op != DRE) {
+ Diag(OpLoc, diag::err_parens_pointer_member_function)
+ << Op->getSourceRange();
+ }
----------------
================
Comment at: clang/lib/Sema/SemaExpr.cpp:14896-14897
+ // Taking the address of a dtor is illegal per C++ [class.dtor]p2.
+ if (isa<CXXDestructorDecl>(MD))
+ Diag(OpLoc, diag::err_typecheck_addrof_dtor) << DRE->getSourceRange();
+
----------------
================
Comment at: clang/lib/Sema/SemaExpr.cpp:14899-14900
+
+ if (DRE->getQualifier())
+ return true;
+ if (MD->getParent()->getName().empty())
----------------
================
Comment at: clang/lib/Sema/SemaExpr.cpp:14901-14911
+ if (MD->getParent()->getName().empty())
+ Diag(OpLoc, diag::err_unqualified_pointer_member_function)
+ << DRE->getSourceRange();
+ else {
+ SmallString<32> Str;
+ StringRef Qual = (MD->getParent()->getName() + "::").toStringRef(Str);
+ Diag(OpLoc, diag::err_unqualified_pointer_member_function)
----------------
================
Comment at: clang/lib/Sema/SemaExpr.cpp:14909
+ << DRE->getSourceRange()
+ << FixItHint::CreateInsertion(DRE->getSourceRange().getBegin(), Qual);
+ }
----------------
Test coverage for the fix-it?
================
Comment at: clang/lib/Sema/SemaExpr.cpp:15043-15046
+ } else if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(op)) {
+ CXXMethodDecl *MD = dyn_cast_or_null<CXXMethodDecl>(DRE->getDecl());
+ if (MD)
+ DiagnoseMissingQualifiersInAddressOfOperand(OpLoc, OrigOp.get(), MD);
----------------
================
Comment at: clang/lib/Sema/SemaExpr.cpp:20550-20554
+static void FixDependencyOfIdExpressionsInLambdaWithDependentObjectParameter(
+ Sema &SemaRef, ValueDecl *D, Expr *E) {
+ DeclRefExpr *ID = dyn_cast<DeclRefExpr>(E);
+ if (!ID || ID->isTypeDependent())
+ return;
----------------
================
Comment at: clang/lib/Sema/SemaExpr.cpp:20556
+ auto IsDependent = [&]() {
+ auto *LSI = SemaRef.getCurLambda();
+ if (!LSI)
----------------
Please spell out the type explicitly. Also, can this be a const pointer?
================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:1395-1404
+ if (ThisTy.isNull()) {
+ DeclContext *DC = getFunctionLevelDeclContext();
+ if (CXXMethodDecl *method = dyn_cast<CXXMethodDecl>(DC);
+ method && method->isExplicitObjectMemberFunction()) {
+ return Diag(Loc, diag::err_invalid_this_use) << 1;
+ } else if (isLambdaCallWithExplicitObjectParameter(CurContext)) {
+ return Diag(Loc, diag::err_invalid_this_use) << 1;
----------------
================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:4330-4331
+ ExprResult Res = FixOverloadedFunctionReference(From, Found, Fn);
+ if (Res.isInvalid())
+ return ExprError();
----------------
Do you have test coverage for this change or is it an NFC change?
================
Comment at: clang/lib/Sema/SemaExprMember.cpp:240
+ if (!Replacement.empty())
+ Diag << FixItHint::CreateInsertion(Loc, Replacement);
+ } else if (ContextClass && RepClass && SS.isEmpty() &&
----------------
Test coverage for the fix-it?
================
Comment at: clang/lib/Sema/SemaExprMember.cpp:259
+ if (!Replacement.empty())
+ Diag << FixItHint::CreateInsertion(Loc, Replacement);
+ }
----------------
Test coverage for the fix-it?
================
Comment at: clang/lib/Sema/SemaLambda.cpp:393-394
+ CXXRecordDecl *RD = Method->getParent();
+ if (Method->isDependentContext())
+ return;
+ if (RD->getLambdaCaptureDefault() == LambdaCaptureDefault::LCD_None &&
----------------
Do we care that the method is a dependent context? I thought we'd be checking if the method itself is dependent?
================
Comment at: clang/lib/Sema/SemaOverload.cpp:1286-1287
return true;
+ CXXMethodDecl *OldMethod = dyn_cast<CXXMethodDecl>(Old);
+ CXXMethodDecl *NewMethod = dyn_cast<CXXMethodDecl>(New);
+
----------------
================
Comment at: clang/lib/Sema/SemaOverload.cpp:1292-1293
+
+ // When determining if a method is an overload from a
+ // base class, act as if the implicit object parameter are of the same type
+
----------------
================
Comment at: clang/lib/Sema/SemaOverload.cpp:1295
+
+ auto NormalizeQualifiers = [&](CXXMethodDecl *M, Qualifiers Q) {
+ if (M->isExplicitObjectMemberFunction())
----------------
================
Comment at: clang/lib/Sema/SemaOverload.cpp:1299-1300
+
+ // We do not allow overloading based off of '__restrict'.
+ Q.removeRestrict();
+
----------------
Just restrict? So `_Nonnull` is fine?
================
Comment at: clang/lib/Sema/SemaOverload.cpp:1324
+ OldMethod->getParent() != NewMethod->getParent()) {
+ auto ParentType =
+ Context.getTypeDeclType(OldMethod->getParent()).getCanonicalType();
----------------
================
Comment at: clang/lib/Sema/SemaOverload.cpp:1382
+
+ auto IsImplicitWithNoRefQual = [](CXXMethodDecl *F) -> bool {
+ return F->getRefQualifier() == RQ_None &&
----------------
================
Comment at: clang/lib/Sema/SemaOverload.cpp:1387-1392
+ if (IsImplicitWithNoRefQual(Old) != IsImplicitWithNoRefQual(New)) {
+ if (CompareType(OldObjectType.getNonReferenceType(),
+ NewObjectType.getNonReferenceType())) {
+ return true;
+ }
+ }
----------------
================
Comment at: clang/lib/Sema/SemaOverload.cpp:1397
+ if (!HaveCorrespondingObjectParameters) {
+ DiagnoseInconsistentRefQualifiers();
+ // CWG2554
----------------
Should you be looking at the return value here?
================
Comment at: clang/lib/Sema/SemaOverload.cpp:1401-1405
+ if (UseOverrideRules && (NewMethod->isExplicitObjectMemberFunction() ||
+ OldMethod->isExplicitObjectMemberFunction()))
+ ;
+ else
+ return true;
----------------
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