[PATCH] D79714: [Diagnostics] Restore -Wdeprecated warning when user-declared copy assignment operator is defined as deleted (PR45634)

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:158
+def DeprecatedCopy : DiagGroup<"deprecated-copy", [DeprecatedCopyUserProvided]>;
+def DeprecatedCopyDtor : DiagGroup<"deprecated-copy-dtor", [DeprecatedCopyDtorUserProvided]>;
 def DeprecatedDeclarations : DiagGroup<"deprecated-declarations">;
----------------
xbolva00 wrote:
> Quuxplusone wrote:
> > If we're going to provide these options at all, I think it would be more grammatical to call them `-Wdeprecated-copy-with-deleted-copy` and `-Wdeprecated-copy-with-deleted-dtor`.
> > 
> > The existing code is already confusing by talking about a "copy dtor" as if that's a thing; I think it makes it even worse to talk about "deprecated copy user provided," since the actually deprecated thing is the //implicitly generated// copy.
> > 
> > I get that we're trying to be terse, and also somewhat hierarchical in putting all the `-Wdeprecated-copy`-related warnings under `-Wdeprecated-copy-foo-bar`; but the current names are too far into the ungrammatical realm for me personally.
> Yeah, better names are welcome :)
Do the current names match existing practice in GCC or anything?
I continue to opine that these options are poorly named. My best suggestion is
`deprecated-copy`, `deprecated-copy-with-dtor`, `deprecated-copy-with-deleted-copy`, `deprecated-copy-with-deleted-dtor` — operating on the (wrong?) assumption that absolutely the only difference between "user-declared" and "user-provided" corresponds to "user-declared as deleted."

Even if everything else remains the same, the internal identifier `warn_deprecated_copy_dtor_operation_user_provided` should certainly be `warn_deprecated_copy_operation_dtor_user_provided` — the phrase that goes together is "copy operation", not "dtor operation".

Your "deprecated-dtor-user-provided.cpp" passes `-Wdeprecated-copy-dtor-user-provided`, but the message text talks about "user-//declared//." Shouldn't the spelling of the option reflect the wording of the message text? This isn't important from the POV of someone who's just going to look at the message text and fix their code, but it's important from the POV of someone who's going to use `-Wno-` and wants to know exactly which bad situations they're ignoring. (Also, the name of the file should match the spelling of the option.)


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13948
+    S.Diag(UserDeclaredOperation->getLocation(), DiagID)
+        << RD << /*copy assignment*/ IsCopyAssignment;
   }
----------------
The `/* copy assignment */` comment is probably no longer useful.


================
Comment at: clang/test/SemaCXX/deprecated-copy.cpp:7
+#ifdef NO_USER_PROVIDED
+// expected-no-diagnostics
+#endif
----------------
Quuxplusone wrote:
> xbolva00 wrote:
> > xbolva00 wrote:
> > > Quuxplusone wrote:
> > > > I'm fairly confident this should just be two different test files. Also, if a test file has an un-ifdeffed `// expected-no-diagnostics` plus an un-ifdeffed `// expected-note ...`, which one wins?
> > > ifdef is here
> > > 
> > > and ifNdef is below :)
> > and DEPRECATED_COPY_DTOR is in own "code block"
> Ah, you're right, I had missed that line 18 was still controlled by the `#ifdef DEPRECATED_COPY_DTOR` condition.
> I still think this should be three(!) different files. Line 2 doesn't even look right to me: shouldn't it be `-Wdeprecated-copy -Wno-deprecated-copy-user-provided`?
> 
> I just did a `git grep 'RUN:' | grep '[-]Wno-' | grep -v '[-]W[^n]'` and found a massive number of tests that put `-Wno-foo` on the command line without putting `-Wbar` anywhere on the same line; I suspect many of these are bugs.
I've lost track of what I was trying to say here and whether I was right ;) but I assume we can consider this comment thread "resolved"?


================
Comment at: clang/test/SemaCXX/deprecated-dtor-user-provided.cpp:6
+  int *ptr;
+  ~A(); // expected-warning {{definition of implicit copy constructor for 'A' is deprecated because it has a user-declared destructor}}
+};
----------------
Do you have a test case for when an operation is defaulted as deleted?
I actually don't understand Clang's/GCC's current behavior in this area.
https://godbolt.org/z/9h3qqP
Of these three situations, do //any// of them rely on deprecated behavior?
(And vice versa, what if we delete the copy constructor and try to use the implicit copy-assignment operator?)


================
Comment at: clang/test/SemaCXX/deprecated.cpp:108
+  };                            // expected-warning at -1 {{definition of implicit copy assignment operator for 'DefaultedDtor' is deprecated because it has a user-declared destructor}}
+  DefaultedDtor d1, d2(d1);     // expected-note {{in implicit copy constructor for 'DeprecatedCopy::DefaultedDtor' first required here}}
+  void h() { d1 = d2; }         // expected-note {{in implicit copy assignment operator for 'DeprecatedCopy::DefaultedDtor' first required here}}
----------------
Nit: I would prefer to see the declarations of d1 and d2 on individual lines, both for style and to prove that d1's declaration triggers no warnings.


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

https://reviews.llvm.org/D79714