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

[Corentin Jabot via Phabricator via llvm-commits]

cor3ntin marked 2 inline comments as done.
cor3ntin added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7260-7265
+def ext_deducing_this : ExtWarn<
+  "explicit object parameters are a C++2b extension">,
+  InGroup<CXX23>;
+def warn_cxx20_ext_deducing_this  : Warning<
+  "explicit object parameters are incompatible with C++ standards before C++2b">,
+  DefaultIgnore, InGroup<CXXPre23Compat>;
----------------
aaron.ballman wrote:
> Missing test coverage for both of these.
> 
> What is the rationale for exposing this as an extension in earlier language modes instead of leaving this a C++26 and later feature?
Can you clarify, what do you think is missing test coverage?


================
Comment at: clang/lib/Sema/SemaOverload.cpp:1397
+    if (!HaveCorrespondingObjectParameters) {
+      DiagnoseInconsistentRefQualifiers();
+      // CWG2554
----------------
aaron.ballman wrote:
> Should you be looking at the return value here?
Might as well. Not sure it changes anything.


================
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:
> 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...


================
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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140828