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

[Corentin Jabot via Phabricator via llvm-commits]

cor3ntin added inline comments.


================
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}}
+};
----------------
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


================
Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:19
+
+    void g(this auto) const; // expected-error{{a function with an explicit object parameter cannot be const}}
+    void h(this auto) &; // expected-error{{a function with an explicit object parameter cannot be reference-qualified}}
----------------
aaron.ballman wrote:
> We've got an inconsistency with our diagnostic wording; this one says `const` explicitly, but the other ones say `have qualifiers`. Should these be unified?
You prefer "const-qualified"?


================
Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:28
+    void variadic(this auto...); // expected-error{{an explicit object parameter cannot be a function parameter pack}}
+    void not_first(int, this auto); // expected-error {{an explicit object parameter can only appear as the first parameter of the function}}
+
----------------
aaron.ballman wrote:
> Should we add a fix-it for this situation or is that overkill?
What did the user intend if they put it there? It seems overkill.


================
Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:49
+    int h(this B&&); // expected-error {{an explicit object parameter cannot appear in a virtual function}}
+    int h(this const B&&); // expected-error {{an explicit object parameter cannot appear in a virtual function}}
+    int h(this A&); // expected-error {{an explicit object parameter cannot appear in a virtual function}}
----------------
aaron.ballman wrote:
> Should this hide the other virtual function? Isn't this morally equivalent to:
> ```
> struct B {
>   virtual void func();
> };
> 
> struct D : B {
>   void func() const;
> };
> ```
> https://godbolt.org/z/ja8Mx9aaE
Same reply as below


================
Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:51
+    int h(this A&); // expected-error {{an explicit object parameter cannot appear in a virtual function}}
+    int h(this int); // expected-error {{an explicit object parameter cannot appear in a virtual function}}
+};
----------------
aaron.ballman wrote:
> This is not a virtual function, it would hide the virtual function in this case, wouldn't it?
Per wg21.link/CWG2554

If a virtual member function F is declared in a class B, and, in a class D derived (directly or indirectly) from B, a declaration of a member function G corresponds (6.4.1 [basic.scope.scope]) to a declaration of F as if declared in D (12.2.2.1 [over.match.funcs.general]), ignoring trailing requires-clauses, and, if G is an explicit object member function, ignoring object parameters, and, if G is an implicit object member function, F and G have the same ref-qualifier (or absence thereof), then G overrides [ Footnote: ... ] F .

In particular. `if G is an explicit object member function, ignoring object parameters`


================
Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:173
+    [i = 0](this auto){ i++; }();
+    [i = 0](this const auto&){ i++; }();
+    // expected-error at -1 {{cannot assign to a variable captured by copy in a non-mutable lambda}}
----------------
aaron.ballman wrote:
> How about:
> ```
> [i = 0](this const auto &) mutable { i++; }();
> ```
> 
Line 10


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140828