[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
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:
> > 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`
Oh, I was thinking of something different than what you did. Can we do:
```
bool IsOverload(FunctionDecl *New, FunctionDecl *Old,
                  bool UseMemberUsingDeclRules, bool ConsiderCudaAttrs = true);
bool IsOverride(FunctionDecl *MD, FunctionDecl *BaseMD);
```
in the header file, and then in the implementation we dispatch to the same static helper function?

(What I was hoping to get rid of was `UseOverrideRules` from the public signature entirely so callers don't have to wonder what that means.)


================
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());
+  }
----------------
cor3ntin wrote:
> 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?
Whatever one exercises this code path. :-D I *think* that's going to be:
```
struct S {
  void foo(this S&&);
};

void func() {
  S{}.foo();
}
```
but am not 100% sure.


================
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:
> 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
No need to add it in that case.


================
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:
> > 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?
How about we split it into two diagnostics:
```
static void h() {
  f(); // expected-error{{call to non-static member function without an object argument}}
  f(Over_Call_Func_Example{});   // expected-error{{cannot pass an explicit object as an argument to the call; did you mean to use '.'?}}
  Over_Call_Func_Example{}.f(); // ok
}
```
the idea being: if the call is to an explicit object function and the user provided an extra argument to the call whose type and position matches the explicit object parameter, we tell the users "that's now how you use this" and best-case give them a fix-it to move the argument to before the call and add a `.`?


================
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:
> > 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
Okay, when we land this, can you file an issue so we don't lose track of the improvement?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828