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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
