[PATCH] D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D30610#934891, @malcolm.parsons wrote:

> I have 1000s of warnings from this check.
>
> A lot of them are about google tests:
>  warning: class 'Foo_Bar_Test' defines a copy constructor and a copy assignment operator but does not define a destructor [cppcoreguidelines-special-member-functions]
>  I'd like an option to suppress these.
>
> It warns about classes that use boost::noncopyable:
>  warning: class 'Foo' defines a non-default destructor but does not define a copy constructor or a copy assignment operator [cppcoreguidelines-special-member-functions]
>  class Foo : boost::noncopyable
>  I think this is a bug in the check.


It is not a bug in the check; the check implements the C++ Core Guideline rule. Hence the comment about discussing it with them.

I think your use cases are compelling and might warrant an option. However, with that in mind combined with `AllowMissingMoveFunctions`, it makes me think this check should not have any of the options -- it's a sign that your code base isn't ready to comply with C.21 in C++ Core Guidelines. The wording for the guideline is pretty specific about what the authors intend: "Compilers enforce much of this rule and ideally warn about any violation." and its enforcement "(Simple) A class should have a declaration (even a =delete one) for either all or none of the special functions." I'm not saying we shouldn't add this option you're looking for, but I'm still a bit uncomfortable with coming up with option combinations that effectively disable the entire point to a check in a coding standard -- that's why we allow you to disable the check more visibly.

> In https://reviews.llvm.org/D30610#934862, @fgross wrote:
> 
>> My 2 cents on this one: Even with `AllowMissingMoveFunctions=1` at least the missing destructor definition should be diagnosed, because it violates the classic rule of three. If you delete your copy operations, you likely have some resources that need to be taken care of in your destructor, so this piece of code would worry me. Better be clear about it and explicitly default the destructor.
> 
> 
> This is a rule of zero class, but with copying disabled; the resources are just as safe as with a normal rule of zero class.

Depending on the resource; you might allocate a resource in the constructor, deleted copy (and thus no move) operators, but still want to release the resource in the destructor.


https://reviews.llvm.org/D30610