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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Ping @arthur.j.odwyer



================
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">;
----------------
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.


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

https://reviews.llvm.org/D79714