[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#934704, @malcolm.parsons wrote:

> In https://reviews.llvm.org/D30610#934617, @aaron.ballman wrote:
>
> > In https://reviews.llvm.org/D30610#934452, @malcolm.parsons wrote:
> >
> > > I'd like an `AllowDeletedCopyFunctions` option that allows move and destructor functions to be missing when copying is disabled.
> > >
> > >   struct A {
> > >     A(const A&) = delete;
> > >     A& operator=(const A&) = delete;
> > >   }
> > >   
> >
> >
> > Doesn't `AllowMissingMoveFunctions` do almost that? If not, it should -- that code produces a class that does not declare a move constructor or move assignment operator per [class.copy]p8 and [class.copy.assign]p4, so that would be a "missing move function". Granted, that doesn't handle the dtor case, but I think an `AllowMissingDestructor` option might be overkill -- the destructor isn't missing, it's implicitly declared as defaulted in that case, but if the C++ Core Guidelines folks want it spelled out explicitly then, it might be worth the option. Have they weighed in on your exception?
>
>
> The check is about providing a consistent set of special member functions.


Understood.

> If you've written a destructor then you probably need to write copy functions to avoid double free.

Sometimes. RAII is a good example where that's not always true. However, in the general case, I agree.

> If you've defaulted the destructor, as often needed in a base class, then the default copy functions are still valid, so `AllowSoleDefaultDtor` makes sense.

Agreed.

> If you've written your own copy functions then you probably want to write your own move functions to allow moving, so `AllowMissingMoveFunctions` doesn't make sense.

I disagree. Move constructors can gracefully degenerate into a copy operation when the copy and move semantics are identical. Think about classes that only contain scalar values as a common example of this. `AllowMissingMoveFunctions` is the option which serves that common purpose.

> If you've deleted the copy functions, then you probably don't want the move functions either, so `AllowDeletedCopyFunctions` would make sense.

I agree with the premise, but disagree with your conclusion. If you've deleted the copy functions *you don't have the move functions* unless you explicitly spell them out (they are not implicitly generated for you). To that end, I don't think this check should diagnose the lack of move operations in the presence of deleted copy operations. However, I'd like to know what the Core Guidelines people think about this scenario. If the copy operations are explicitly deleted, the primary difference between the default behavior and explicitly deleted move operations is that the diagnostics change from "no such operator" to "explicitly deleted operator". Do they think that's worth the extra typing for the user?


https://reviews.llvm.org/D30610