[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 24 12:04:40 PDT 2023
aaron.ballman added a reviewer: erichkeane.
aaron.ballman added a comment.
Phew, that completes my first pass through the review! I'm also adding @erichkeane as a reviewer now that he's off sabbatical.
================
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 "
----------------
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?
================
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}}
+};
----------------
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
}
```
================
Comment at: clang/test/CXX/drs/dr26xx.cpp:1
-// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-unknown %s -verify
+// RUN: %clang_cc1 -std=c++20 -Wno-c++2b-extensions -triple x86_64-unknown-unknown %s -verify
+// RUN: %clang_cc1 -std=c++2b -triple x86_64-unknown-unknown %s -verify
----------------
Do we need `-Wno-c++2b-extensions`? All the changes in the file are protected by c++23 version checks.
================
Comment at: clang/test/CXX/drs/dr26xx.cpp:125
+#if __cplusplus >= 202302L
+namespace dr2653 { // dr2653: 16
+ struct Test { void f(this const auto& = Test{}); };
----------------
You should update the other comments as well. :-)
================
Comment at: clang/test/CXX/drs/dr26xx.cpp:128
+ // expected-error at -1 {{an explicit object parameter cannot have a default argument}}
+ auto L =[](this const auto& = Test{}){};
+ // expected-error at -1 {{an explicit object parameter cannot have a default argument}}
----------------
================
Comment at: clang/test/CXX/drs/dr26xx.cpp:176
+void test() {
+ (&S::f)(1); //expected-error {{called object type 'void (dr2687::S::*)(int)' is not a function or function pointer}}
+ (&S::g)(1);
----------------
================
Comment at: clang/test/CXX/over/over.load/p2-0x.cpp:10-13
+class Y {
+ void h() &;
+ void h() const &;
+ void h() &&;
----------------
Spurious whitespace changes, this whole file can be reverted I think.
================
Comment at: clang/test/CXX/special/class.copy/p20.cpp:14
-struct VirtualInheritsNonConstCopy : virtual NonConstCopy {
+struct VirtualInheritsNonConstCopy : virtual NonConstCopy {
VirtualInheritsNonConstCopy();
----------------
Spurious whitespace changes, this whole file can be reverted I think.
================
Comment at: clang/test/CXX/special/class.copy/p25-0x.cpp:115-118
+ TNT &operator=(EXPLICIT_PARAMETER(TNT&&) const TNT &) = default; // trivial
+ TNT &operator=(EXPLICIT_PARAMETER(TNT&&) TNT &); // non-trivial
+ TNT &operator=(EXPLICIT_PARAMETER(TNT&&) TNT &&) = default; // trivial
+ TNT &operator=(EXPLICIT_PARAMETER(TNT&&) const TNT &&); // non-trivial
----------------
Might as well skip using `EXPLICIT_PARAMETER` for these since they're in the guarded block already anyway?
================
Comment at: clang/test/CodeGenCXX/cxx2b-deducing-this.cpp:4
+
+struct TrivialStruct {
+ void explicit_object_function(this TrivialStruct) {}
----------------
I'd like to see more codegen tests in general -- for example, a test that demonstrates we properly handle code like:
```
struct B {
virtual void f();
};
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()
}
```
but also tests that show that we do the correct thing for calling conventions (do explicit object parameter functions act as `__fastcall` functions?), explicit object parameters in lambdas, call through a pointer to member function, and so on.
Another test that could be interesting is how chained calls look (roughly):
```
struct S {
void foo(this const S&);
};
struct T {
S bar(this const &T);
};
void func() {
T t;
t.bar().foo();
}
```
================
Comment at: clang/test/CodeGenCXX/cxx2b-mangle-deducing-this.cpp:1
+// RUN: %clang_cc1 -std=c++2b -fno-rtti -emit-llvm -triple x86_64-linux -o - %s 2>/dev/null | FileCheck %s
+
----------------
Is `-fno-rtti` necessary for some reason?
================
Comment at: clang/test/CodeGenCXX/microsoft-abi-explicit-object-parameters.cpp:1
+// RUN: %clang_cc1 -std=c++2b -fno-rtti -emit-llvm -triple=x86_64-pc-win32 -o - %s 2>/dev/null | FileCheck %s
+struct S {
----------------
Same question here about `-fno-rtti`.
================
Comment at: clang/test/SemaCXX/cxx2b-deducing-this-coro.cpp:1-4
+// RUN: %clang_cc1 -std=c++2b %s -verify
+
+
+#include "Inputs/std-coroutine.h"
----------------
================
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}}
----------------
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?
================
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}}
+
----------------
Should we add a fix-it for this situation or is that 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}}
----------------
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
================
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}}
+};
----------------
This is not a virtual function, it would hide the virtual function in this case, wouldn't it?
================
Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:57
+ struct Test { void f(this const auto& = Test{}); };
+ // expected-error at -1 {{an explicit object parameter cannot have a default argument}}
+ auto L =[](this const auto& = Test{}){};
----------------
I wonder if we want to reword this ever-so-slightly to `the explicit object parameter cannot have a default argument` to help clarify this situation:
```
void f(this const auto & = Test{}, int i = 12);
```
================
Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:58
+ // expected-error at -1 {{an explicit object parameter cannot have a default argument}}
+ auto L =[](this const auto& = Test{}){};
+ // expected-error at -1 {{an explicit object parameter cannot have a default argument}}
----------------
================
Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:62
+
+struct CannotUseThis {
+ int fun();
----------------
I'd like an additional test case:
```
struct B {
void foo();
int n;
static int i;
};
struct D : B {
void bar(this auto) {
foo(); // error
n = 12; // error
i = 100; // Okay, I presume?
}
};
```
================
Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:94
+ DerivedErr dko{ko};
+ dko();
+}
----------------
Other tests I'd like to see are:
```
struct Frobble;
auto nothingIsOkay = [i = 0](this const Frobble &) {};
struct Frobble {} f; // Should cause a diagnostic on the lambda?
nothingIsOkay(f);
```
and
```
auto alsoOk = [](this const Test &) {}; // Fine because there's no capture?
alsoOk(Test{});
```
================
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}}
----------------
How about:
```
[i = 0](this const auto &) mutable { i++; }();
```
================
Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:183
+
+ void f(this const Over_Call_Func_Example&); //expected-note {{here}}
+ void g() const {
----------------
================
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
----------------
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?
================
Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:200
+ c.k(); // ok
+ }
+};
----------------
Another test that might be interesting is:
```
struct S {
void f(this int);
void f(this float);
operator int() const;
operator float() const;
void test(this const S &s) {
s.f(); // Ambiguous call
}
};
```
I'm especially curious how well the notes come out.
Also, what about this?
```
struct S {
void s(this short);
operator int() const;
void test(this const S &val) {
val.s();
}
};
struct T {
void s(this int);
operator short() const;
void test(this const T &val) {
val.s();
}
};
```
to test how implicit conversions factor in.
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