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


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



================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13864
+    S.Diag(UserDeclaredOperation->getLocation(), DiagID)
         << RD << /*copy assignment*/ !isa<CXXConstructorDecl>(CopyOp);
   }
----------------
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;
```


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79714