[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 accepted this revision.
Quuxplusone added a comment.
This revision is now accepted and ready to land.

The new individual test files are awesome. :)
The name of the `-W` options still aren't perfect IMHO, but maybe it will never be perfect, and meanwhile everything else looks great.



================
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:
> > 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.)
> Yeah, deprecated-copy and deprecated-copy-dtor is already defined by GCC.
> 
> I think deprecated-copy-user-provided-copy and deprecated-copy-user-provided-dtor could be good solution here, it is also quite easy to follow implementation of the checking logic in Sema with these names.
@xbolva00: I suggested `-Wdeprecated-copy-with-user-provided-copy`; you've got `-Wdeprecated-copy-user-provided-copy`. Could I persuade you to use the `-with-` versions?
I suppose where it really matters is `-Wdeprecated-copy-dtor` (what is a "copy destructor"?), but that's for GCC compatibility, so we can't change it. Right?


================
Comment at: clang/test/SemaCXX/deprecated-copy-dtor.cpp:2
+// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated -verify
+// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated-copy-dtor -verify
+
----------------
I think it's extremely unfortunate that `-Wdeprecated-copy-dtor` is not a subset of `-Wdeprecated-copy`. But again, GCC-compatibility binds our hands here AFAIK. https://godbolt.org/z/3r3ddvTfG


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

https://reviews.llvm.org/D79714