[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 marked 4 inline comments as done.
xbolva00 added a comment.

Thanks for initial 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">;
----------------
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 :)


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:568
+def warn_deprecated_copy_dtor_operation_user_provided : Warning<
+  warn_deprecated_copy_dtor_operation.Text>,
+  InGroup<DeprecatedCopyDtorUserProvided>, DefaultIgnore;
----------------
Quuxplusone wrote:
> This is the first time I've ever seen this idiom. As a person who often greps the wording of an error message to find out where it comes from, I would very strongly prefer to see the actual string literal here. But if this is established practice, then okay.
> 
I think this is established way how to duplicated warning text strings.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13864
+    S.Diag(UserDeclaredOperation->getLocation(), DiagID)
         << RD << /*copy assignment*/ !isa<CXXConstructorDecl>(CopyOp);
   }
----------------
Quuxplusone wrote:
> I wonder if this would be easier to read as
> 
> ```
> if (UserDeclaredOperation) {
>     bool UDOIsUserProvided = UserDeclaredOperation->isUserProvided();
>     bool UDOIsDestructor = isa<CXXDestructorDecl>(UserDeclaredOperation);
>     bool IsCopyAssignment = !isa<CXXConstructorDecl>(CopyOp);
>     unsigned DiagID =
>         ( UDOIsUserProvided &&  UDOIsDestructor) ? diag::warn_deprecated_copy_dtor_operation_user_provided :
>         ( UDOIsUserProvided && !UDOIsDestructor) ? diag::warn_deprecated_copy_operation_user_provided :
>         (!UDOIsUserProvided &&  UDOIsDestructor) ? diag::warn_deprecated_copy_dtor_operation :
>         diag::warn_deprecated_copy_operation;
>     S.Diag(UserDeclaredOperation->getLocation(), DiagID)
>         << RD << /*copy assignment*/ IsCopyAssignment;
> ```
I have been thinking about similar form, so +1.


================
Comment at: clang/test/SemaCXX/deprecated-copy.cpp:7
+#ifdef NO_USER_PROVIDED
+// expected-no-diagnostics
+#endif
----------------
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 :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79714