[PATCH] D16376: clang-tidy check: rule-of-five

[David Blaikie via cfe-commits]

On Wed, Feb 3, 2016 at 1:40 PM, Aaron Ballman <aaron.ballman at gmail.com>
wrote:

> On Wed, Feb 3, 2016 at 4:13 PM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> > On Wed, Feb 3, 2016 at 1:03 PM, Jonathan Coe <jon at jbcoe.net> wrote:
> >>
> >>
> >>
> >> On 3 February 2016 at 18:44, David Blaikie <dblaikie at gmail.com> wrote:
> >>>
> >>>
> >>>
> >>> On Wed, Feb 3, 2016 at 10:23 AM, Jonathan Coe <jon at jbcoe.net> wrote:
> >>>>
> >>>> All the C++ compilers I have tried using (GCC,Clang,MSVC) will
> generate
> >>>> assignment operators even if the user defines a copy-constructor.
> This is
> >>>> the behaviour I set out to write a check for.
> >>>>
> >>>> The cpp core guide lines recommend defining all or none of the special
> >>>> functions
> >>>>
> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c21-if-you-define-or-delete-any-default-operation-define-or-delete-them-all
> >>>>
> >>>> If the deprecation warning you mention will turn off the deprecated
> >>>> generation of special member functions when others are defined (or
> warn when
> >>>> deprecated compiler-generated functions are used) then the
> rule-of-five
> >>>> check is of reduced value.
> >>>
> >>>
> >>> It can't stop them being generated, because that's required behavior -
> >>> warnings don't change the program behavior.
> >>>
> >>
> >> That's true but promoting them to errors will stop compilation and
> prevent
> >> the sort of bug I'm trying to stop.
> >
> >
> > Sure - and the user can do that with -Werror=deprecated (but, yes, we
> should
> > split out that specific deprecation so it can be turned on without
> turning
> > on other deprecation)
> >
> >>
> >>
> >>>
> >>> Wording like this is in the C++11 standard:
> >>>
> >>> "If the class definition does not explicitly declare a copy
> constructor,
> >>> one is declared implicitly. If the class definition declares a move
> >>> constructor or move assignment operator, the implicitly declared copy
> >>> constructor is defined as deleted; otherwise, it is defined as
> defaulted
> >>> (8.4). The latter case is deprecated if the class has a user-declared
> copy
> >>> assignment operator or a user-declared destructor."
> >>>
> >>
> >> The 'deprecated' part is my target. I've seen code with user-defined
> copy
> >> constructors behave badly when compiler-generated assignment operators
> are
> >> unwittingly used.
> >
> >
> > For sure - because it's only deprecated, not an error. Clang-tidy won't
> > change that - it still won't be a hard error in every codebase.
> >
> > I agree that having all the Core Guidelines checks in one place is a good
> > idea, so I'm not making any real suggestion here, sorry - just that it
> seems
> > unfortunate to relegate this check (& encourage explicit & mostly
> redundant
> > defaulting/deleting) to a separate tool when it's essentially built into
> the
> > language and compiler already. My disagreement is perhaps more with the
> Core
> > Guideline than with its implementation here.
> >
> >>
> >> The rule of five lets me locally reason about code without having to
> worry
> >> (needlessly or not) about whether some functions are potentially being
> >> compiler-generated.
> >
> >
> > But once the language does the right thing directly, rather than
> providing a
> > clang-tidy warning that encourages you to change the code to achieve the
> > same result, why would we worry about being explicit?
>
> It could be argued that implicit code generated by the compiler is
> magic, and being explicit is a readability improvement. You no longer
> have to think "under what circumstances is this generated?"


Except this rule doesn't argue that - if all the members are implicit that
is acceptable to this rule (even if the implicit special members are
entirely non-trivial because one of the member variables has non-trivial
special members). The issue is about implicit code when at least one of
them is explicit.

Except that doesn't happen in C++11 (except in the two deprecated cases) -
there is no implicit code to be worried about except for historic (and
deprecated - I agree, we could do better there*) reasons. New C++
programmers won't need to be taught the rule of 5 - the language will just
DTRT. I'm not sure it's necessarily an improvement to create coding
conventions for that sort of historic reason.

* but 'better' might just be separating out that deprecation warning and
enabling it as a clang-tidy warning in the core guidelines group

- Dave
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160203/e7661000/attachment.html>