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