[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)
Corentin Jabot via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 27 14:05:53 PDT 2023
cor3ntin added inline comments.
================
Comment at: clang/include/clang/AST/ASTLambda.h:45
+ return isLambdaCallOperator(DC) &&
+ !cast<CXXMethodDecl>(DC)->getType().isNull() &&
+ !cast<CXXMethodDecl>(DC)->isExplicitObjectMemberFunction();
----------------
aaron.ballman wrote:
> 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?
I added a fixme.
I spent some time thinking about alternatives but i can't think of something obvious at the moment.
Making a lambda being parsed invalid does not seem ideal either
================
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:
> 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.
Discussed offline, we can keep those
================
Comment at: clang/include/clang/Sema/Sema.h:3775
bool UseMemberUsingDeclRules, bool ConsiderCudaAttrs = true,
- bool ConsiderRequiresClauses = true);
+ bool UseOverrideRules = false);
----------------
aaron.ballman wrote:
> 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.
I added `IsOverride`
================
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:
> 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.
Discussed offline, we can keep these here
================
Comment at: clang/lib/Sema/SemaOverload.cpp:911-915
if (X->getNumParams() != Y->getNumParams())
return false;
for (unsigned I = 0; I < X->getNumParams(); ++I)
if (!Ctx.hasSameUnqualifiedType(X->getParamDecl(I)->getType(),
Y->getParamDecl(I)->getType()))
----------------
cor3ntin wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > Do we need changes here?
> > Yes, although I need to figure out a test
> We need changes.
> Which changes is not obvious to me. https://lists.isocpp.org/core/2023/08/14711.php
Added link to https://cplusplus.github.io/CWG/issues/2797.html
================
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:
> aaron.ballman wrote:
> > Do we have test coverage for this situation?
> Still wondering about test coverage for this bit.
What specific test would you like to see?
================
Comment at: clang/lib/Sema/SemaOverload.cpp:1299-1300
+
+ // We do not allow overloading based off of '__restrict'.
+ Q.removeRestrict();
+
----------------
aaron.ballman wrote:
> 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?
Nope, I will add a fixme
================
Comment at: clang/test/CXX/drs/dr25xx.cpp:104-106
+struct D : B {
+ void f(this D&); // expected-error {{an explicit object parameter cannot appear in a virtual function}}
+};
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > I'd like to see two other tests:
> > ```
> > struct D2 : B {
> > void f(this B&); // expected-error {{an explicit object parameter cannot appear in a virtual function}}
> > };
> > ```
> > to demonstrate we also catch it when naming the base class instead of the derived class. And:
> > ```
> > 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(), probably as a codegen test
> > }
> > ```
> I might need help with the codegen test setup, it's sill my nemesis. Or we can put it somewhere else
Added the first test. the second test is still an override. I guess i can still add it but it is IF
================
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
----------------
aaron.ballman wrote:
> 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.
You have a rewording suggestion?
================
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}}
+};
----------------
aaron.ballman wrote:
> 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?
FYI i remember looking at that and i think it was non trivial, I'd rather punt it
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