This patch implements P0847R7 (partially),
CWG2561 and CWG2653.
Details
- Reviewers
shafik NoQ aaron.ballman erichkeane rjmccall efriedma - Group Reviewers
Restricted Project - Commits
- rGaf4751738db8: [C++] Implement "Deducing this" (P0847R7)
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/lib/Sema/SemaOverload.cpp | ||
---|---|---|
15260–15263 | ||
15279 | ||
15516 | ||
15967–15970 |
clang/include/clang/Sema/Sema.h | ||
---|---|---|
3775 | 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. | |
clang/lib/Sema/SemaOverload.cpp | ||
911–915 | Yes, although I need to figure out a test | |
996–999 | Yes, although I need to figure out a test | |
2850–2854 | I've elected not to modify any of the Objective C code paths. I have no idea how Objective c++ inherit new features nor how deducing this would impact it. | |
3174–3176 | What would be the type of Reversed ? llvm::reverse(Old) : Old ? there is no common type between the two branches afaict | |
6254 | Here the first line of the function would have to be Expr* Local = Obj; and then Obj would not be used again. I'm keeping it, lest you insist :) | |
15967–15970 | Oh wow, I didn't spot the gnarly double conversion here. Thanks! |
clang/lib/Sema/SemaOverload.cpp | ||
---|---|---|
911–915 | We need changes. | |
996–999 | UGH, phab ate my comment... if the operator has 2 non object parameters, it is implied that it is a non-member binary operator |
Phew, that completes my first pass through the review! I'm also adding @erichkeane as a reviewer now that he's off sabbatical.
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
9378–9381 | 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? | |
clang/test/CXX/drs/dr25xx.cpp | ||
104–106 | 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 } | |
clang/test/CXX/drs/dr26xx.cpp | ||
1–3 | Do we need -Wno-c++2b-extensions? All the changes in the file are protected by c++23 version checks. | |
125 | You should update the other comments as well. :-) | |
128 | ||
176 | ||
clang/test/CXX/over/over.load/p2-0x.cpp | ||
10–14 | Spurious whitespace changes, this whole file can be reverted I think. | |
clang/test/CXX/special/class.copy/p20.cpp | ||
14 | Spurious whitespace changes, this whole file can be reverted I think. | |
clang/test/CXX/special/class.copy/p25-0x.cpp | ||
115–118 | Might as well skip using EXPLICIT_PARAMETER for these since they're in the guarded block already anyway? | |
clang/test/CodeGenCXX/cxx2b-deducing-this.cpp | ||
5 | 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(); } | |
clang/test/CodeGenCXX/cxx2b-mangle-deducing-this.cpp | ||
2 | Is -fno-rtti necessary for some reason? | |
clang/test/CodeGenCXX/microsoft-abi-explicit-object-parameters.cpp | ||
2 | Same question here about -fno-rtti. | |
clang/test/SemaCXX/cxx2b-deducing-this-coro.cpp | ||
2–5 | ||
clang/test/SemaCXX/cxx2b-deducing-this.cpp | ||
20 | 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? | |
29 | Should we add a fix-it for this situation or is that overkill? | |
50 | Should this hide the other virtual function? Isn't this morally equivalent to: struct B { virtual void func(); }; struct D : B { void func() const; }; | |
52 | This is not a virtual function, it would hide the virtual function in this case, wouldn't it? | |
58 | 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); | |
59 | ||
63 | 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? } }; | |
95 | 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{}); | |
174 | How about: [i = 0](this const auto &) mutable { i++; }(); | |
184 | ||
193 | 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? | |
201 | 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. |
Address some comments (typos, add tests, reword diags)
clang/test/CXX/drs/dr25xx.cpp | ||
---|---|---|
104–106 | I might need help with the codegen test setup, it's sill my nemesis. Or we can put it somewhere else | |
clang/test/SemaCXX/cxx2b-deducing-this.cpp | ||
20 | You prefer "const-qualified"? | |
29 | What did the user intend if they put it there? It seems overkill. | |
50 | Same reply as below | |
52 | 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 | |
174 | Line 10 |
Reuse the logic/diag message diagnosing qualifiers
of static member functions, instead of rolling out
an ad-hoc bit of code.
clang/test/SemaCXX/cxx2b-deducing-this.cpp | ||
---|---|---|
20 | I'm now reusing the same code path we had for static, and same diagnostic message |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
7260–7265 | Can you clarify, what do you think is missing test coverage? | |
clang/lib/Sema/SemaOverload.cpp | ||
1397 | Might as well. Not sure it changes anything. | |
clang/test/SemaCXX/cxx2b-deducing-this.cpp | ||
30–31 | Should we simply remove the newly added diagnostic then? | |
193 | UGH, phab is confused, I'm no longer sure which diag you are concerned about... |
clang/lib/Sema/SemaOverload.cpp | ||
---|---|---|
13037 | I don't think FixOverloadedFunctionReference can ever be null, and if it can, maybe it's better to let it assert so that we know we missed something, right? |
I checked out the code to see how does the Static Analyzer work after this.
I'm impressed that it seems to work.
Do you mind adding my test file to this patch?
clang/test/Analysis/cxx2b-deducing-this.cpp:
// RUN: %clang_analyze_cc1 -std=c++2b -verify %s \ // RUN: -analyzer-checker=core,debug.ExprInspection template <typename T> void clang_analyzer_dump(T); struct S { int num; S *orig; void a(this auto Self) { clang_analyzer_dump(&Self); // expected-warning {{&Self}} clang_analyzer_dump(Self.orig); // expected-warning {{&s}} clang_analyzer_dump(Self.num); // expected-warning {{5 S32b}} clang_analyzer_dump(Self.orig->num); // expected-warning {{5 S32b}} Self.num = 1; clang_analyzer_dump(Self.num); // expected-warning {{1 S32b}} clang_analyzer_dump(Self.orig->num); // expected-warning {{5 S32b}} } void b(this auto& Self) { clang_analyzer_dump(&Self); // expected-warning {{&s}} clang_analyzer_dump(Self.orig); // expected-warning {{&s}} clang_analyzer_dump(Self.num); // expected-warning {{5 S32b}} clang_analyzer_dump(Self.orig->num); // expected-warning {{5 S32b}} Self.num = 2; clang_analyzer_dump(Self.num); // expected-warning {{2 S32b}} clang_analyzer_dump(Self.orig->num); // expected-warning {{2 S32b}} } void c(this S Self) { clang_analyzer_dump(&Self); // expected-warning {{&Self}} clang_analyzer_dump(Self.orig); // expected-warning {{&s}} clang_analyzer_dump(Self.num); // expected-warning {{2 S32b}} clang_analyzer_dump(Self.orig->num); // expected-warning {{2 S32b}} Self.num = 3; clang_analyzer_dump(Self.num); // expected-warning {{3 S32b}} clang_analyzer_dump(Self.orig->num); // expected-warning {{2 S32b}} } void c(this S Self, int I) { clang_analyzer_dump(I); // expected-warning {{11 S32b}} clang_analyzer_dump(&Self); // expected-warning {{&Self}} clang_analyzer_dump(Self.orig); // expected-warning {{&s}} clang_analyzer_dump(Self.num); // expected-warning {{2 S32b}} clang_analyzer_dump(Self.orig->num); // expected-warning {{2 S32b}} Self.num = 4; clang_analyzer_dump(Self.num); // expected-warning {{4 S32b}} clang_analyzer_dump(Self.orig->num); // expected-warning {{2 S32b}} } }; void top() { S s = {/*num=*/5, /*orig=*/&s}; s.a(); s.b(); // This call changes 's.num' to 2. s.c(); s.c(11); }
Thank you for implementing (deducing) this!
Mostly just nits (plus my hatred for C style casts :) ), I didn't see any issues with the approach.
clang/include/clang/AST/ASTLambda.h | ||
---|---|---|
45 | 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? | |
clang/lib/AST/Decl.cpp | ||
3624 | ||
3629 | ||
clang/lib/CodeGen/CGExprCXX.cpp | ||
72 | ||
clang/lib/Sema/SemaDeclCXX.cpp | ||
14942 | These casts should likely not be C-style/clobber casts. | |
14965 | same here | |
15050–15051 | here and elsewhere. |
clang/include/clang/AST/ASTLambda.h | ||
---|---|---|
45 | 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) | |
clang/test/CodeGenCXX/cxx2b-deducing-this.cpp | ||
5 | That first example is ill-formed (it is an override, not allowed) I will need help for codegen tests. |
clang/lib/AST/DeclCXX.cpp | ||
---|---|---|
841 | https://cplusplus.github.io/CWG/issues/2787.html |
clang/test/CodeGenCXX/cxx2b-deducing-this.cpp | ||
---|---|---|
5 | NVM, just different optimization levels |
I think we're getting pretty close! My goal is to get this landed ASAP; I do not think it needs to be hidden behind a feature flag (-std=c++2b is sufficient), and it's good that we're not defining the feature test macro yet. There were a few outstanding questions still that would be good to get answers for. Having some codegen code owner eyes on this would be appreciated, so adding them as reviewers.
clang/include/clang/AST/ASTLambda.h | ||
---|---|---|
45 | 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? | |
clang/include/clang/AST/Decl.h | ||
1722–1724 | spurious whitespace | |
clang/include/clang/AST/DeclCXX.h | ||
2063–2064 | Still needs some docs explaining the difference for folks. | |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
7260–7265 | explicit object parameters are incompatible with C++ standards before C++2b is missing test coverage. | |
7262–7265 | ||
9378–9381 | These changes can still be split off into their own NFC commit. | |
9443–9444 | I don't see any tests modified as a result of this change (and the change seems orthogonal in a similar way as the above changes). | |
clang/include/clang/Sema/Sema.h | ||
3775 |
I think that's a clean solution. | |
clang/lib/AST/ComputeDependence.cpp | ||
502–504 | ||
clang/lib/AST/DeclCXX.cpp | ||
841 |
I think it's fine to do that resolution in a follow-up closely after landing this, unless you want to do it now because it's trivial enough. I don't imagine CWG is going to come back with a drastically different resolution, do you? CC @shafik for opinions on reading CWG tea leaves. | |
2526 | ||
clang/lib/AST/ItaniumMangle.cpp | ||
1729–1731 | Based on the discussion in that thread, I think this direction is reasonable. Ping @rjmccall for any last-minute concerns before we make this part of the ABI. | |
clang/lib/CodeGen/CGExpr.cpp | ||
4274 | ||
clang/lib/Parse/ParseDecl.cpp | ||
5756 | This can still be split off to make the review shorter for folks. | |
clang/lib/Sema/SemaDeclCXX.cpp | ||
11265 | I think this early return should also be pulled. | |
clang/lib/Sema/SemaExpr.cpp | ||
20552 | ||
clang/lib/Sema/SemaOverload.cpp | ||
1299–1300 | Any new thoughts on this? | |
2850–2854 | Objective-C++ is a pure extension over the top of C++, so C++ functionality should behave in the expected way in ObjC++. But because this is with ObjC pointers (which are kind of a special thing), it's not clear to me either. I say let's punt on it; but final word comes from @rjmccall | |
3174–3176 | Oh derp. :-) | |
6254 | I don't insist. | |
6258–6262 | Still wondering about test coverage for this bit. | |
clang/test/CodeGenCXX/cxx2b-deducing-this.cpp | ||
5 |
Oh you're right, I missed a very important "not" in this chain of logic: https://eel.is/c++draft/class#virtual-2.sentence-1 Coupled with the direction in CWG2553 and CWG2554. | |
clang/test/SemaCXX/cxx2b-deducing-this.cpp | ||
30–31 | 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? | |
193 | 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. |
- Nitpicks
- Add more override tests
- Add diagnostic tests for earlier language modes
- Add fixme in areas than need cleaning and unimplemented core issues
clang/include/clang/AST/ASTLambda.h | ||
---|---|---|
45 | I added a fixme. | |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
9378–9381 | Discussed offline, we can keep those | |
clang/include/clang/Sema/Sema.h | ||
3775 | I added IsOverride | |
clang/lib/Parse/ParseDecl.cpp | ||
5756 | Discussed offline, we can keep these here | |
clang/lib/Sema/SemaOverload.cpp | ||
911–915 | Added link to https://cplusplus.github.io/CWG/issues/2797.html | |
1299–1300 | Nope, I will add a fixme | |
6258–6262 | What specific test would you like to see? | |
clang/test/CXX/drs/dr25xx.cpp | ||
104–106 | Added the first test. the second test is still an override. I guess i can still add it but it is IF | |
clang/test/SemaCXX/cxx2b-deducing-this.cpp | ||
30–31 | FYI i remember looking at that and i think it was non trivial, I'd rather punt it | |
193 | You have a rewording suggestion? |
clang/include/clang/Sema/Sema.h | ||
---|---|---|
3775 | 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.) | |
clang/lib/Sema/SemaOverload.cpp | ||
6258–6262 | 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. | |
clang/test/CXX/drs/dr25xx.cpp | ||
104–106 | No need to add it in that case. | |
clang/test/SemaCXX/cxx2b-deducing-this.cpp | ||
30–31 | Okay, when we land this, can you file an issue so we don't lose track of the improvement? | |
193 | 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 .? |
clang/test/SemaCXX/cxx2b-deducing-this.cpp | ||
---|---|---|
193 | @cor3ntin and I discussed this suggestion off-list and came to the conclusion that it would be ideal to improve the error here (and perhaps have a note that points to the this in the declaration of f() + a fixit) but it would be difficult to come up with a heuristic for in all cases. Consider a declaration like void f(this auto, Over_Call_Func_Example); which is called with f(Over_Call_Func_Example{}); from within a static function as a corollary test case to the above. Let's punt on this for now. |
- Add code gen test for materialized temporary
- Split IsOverload in 2 functions
- Fix Previous rebase
Aside from some minor nits in the release notes, I think this LGTM. I'm going to hold off on accepting officially for a day or two just so other code reviews have a chance to weigh in (the codegen code owners were only added to the review today).
clang/docs/ReleaseNotes.rst | ||
---|---|---|
94–98 ↗ | (On Diff #557447) |
Accepting as-is; we'll keep our eyes peeled for any post-commit feedback.
Thank you for all the hard work on this one, it was a big, involved patch!
clang/docs/ReleaseNotes.rst | ||
---|---|---|
94–98 ↗ | (On Diff #557447) | Closing words was not set in this version. have been lost. |
FYI this causes some compile-time regression: http://llvm-compile-time-tracker.com/compare.php?from=bc7d88faf1a595ab59952a2054418cdd0d9eeee8&to=af4751738db89a142a8880c782d12d4201b222a8&stat=instructions:u About 0.7% for C++ code at O0. Sample for a file with >2% would be btSoftSoftCollisionAlgorithm.cpp.
Not sure to what degree this is expected or not.
I think it's at least somewhat expected, but unfortunate. We basically have to check "is this an explicit object argument function or not" in a bunch of places in the compiler and I think that perhaps the extra branching is what's causing the perf situation. I'm not certain what can be done beyond "hook up a profiler and start poking around". Thoughts, @cor3ntin?
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?