xbolva00 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">; ---------------- Quuxplusone wrote: > 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? I can use with “-with” and create alias for -Wdeprecated-copy-dtor to keep gcc compatibility :) 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
