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