Quuxplusone accepted this revision. Quuxplusone added a comment. This revision is now accepted and ready to land.
The new individual test files are awesome. :) The name of the `-W` options still aren't perfect IMHO, but maybe it will never be perfect, and meanwhile everything else looks great. ================ 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">; ---------------- xbolva00 wrote: > Quuxplusone wrote: > > xbolva00 wrote: > > > 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 :) > > Do the current names match existing practice in GCC or anything? > > I continue to opine that these options are poorly named. My best suggestion > > is > > `deprecated-copy`, `deprecated-copy-with-dtor`, > > `deprecated-copy-with-deleted-copy`, `deprecated-copy-with-deleted-dtor` — > > operating on the (wrong?) assumption that absolutely the only difference > > between "user-declared" and "user-provided" corresponds to "user-declared > > as deleted." > > > > Even if everything else remains the same, the internal identifier > > `warn_deprecated_copy_dtor_operation_user_provided` should certainly be > > `warn_deprecated_copy_operation_dtor_user_provided` — the phrase that goes > > together is "copy operation", not "dtor operation". > > > > Your "deprecated-dtor-user-provided.cpp" passes > > `-Wdeprecated-copy-dtor-user-provided`, but the message text talks about > > "user-//declared//." Shouldn't the spelling of the option reflect the > > wording of the message text? This isn't important from the POV of someone > > who's just going to look at the message text and fix their code, but it's > > important from the POV of someone who's going to use `-Wno-` and wants to > > know exactly which bad situations they're ignoring. (Also, the name of the > > file should match the spelling of the option.) > Yeah, deprecated-copy and deprecated-copy-dtor is already defined by GCC. > > I think deprecated-copy-user-provided-copy and > deprecated-copy-user-provided-dtor could be good solution here, it is also > quite easy to follow implementation of the checking logic in Sema with these > names. @xbolva00: I suggested `-Wdeprecated-copy-with-user-provided-copy`; you've got `-Wdeprecated-copy-user-provided-copy`. Could I persuade you to use the `-with-` versions? I suppose where it really matters is `-Wdeprecated-copy-dtor` (what is a "copy destructor"?), but that's for GCC compatibility, so we can't change it. Right? ================ Comment at: clang/test/SemaCXX/deprecated-copy-dtor.cpp:2 +// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated -verify +// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated-copy-dtor -verify + ---------------- I think it's extremely unfortunate that `-Wdeprecated-copy-dtor` is not a subset of `-Wdeprecated-copy`. But again, GCC-compatibility binds our hands here AFAIK. https://godbolt.org/z/3r3ddvTfG 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
