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

[David Blaikie via cfe-commits]

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?


>
> I don't advocate putting this in 'misc'. It belongs in
> 'cppcoreguidelines', I'll move it if the approach taken thus far is sound
> (as discussed in the review).
>
>
>> (similar wording for the copy assignment operator, the dtor has a
>> different/special case if I recall correctly)
>>
>>
>>>
>>> Jon
>>>
>>> On 3 February 2016 at 17:40, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>>> Is this really that useful of a rule? The language does the right thing
>>>> for the most part already (you don't need to explicitly delete them -
>>>> they're implicitly deleted if you define any others - except for backcompat
>>>> with C++98, but those cases are deprecated & we should probably split out
>>>> the warning for that deprecation so it's easier to turn on separately)
>>>>
>>>> On Wed, Feb 3, 2016 at 5:25 AM, Jonathan B Coe via cfe-commits <
>>>> cfe-commits at lists.llvm.org> wrote:
>>>>
>>>>> jbcoe retitled this revision from "clang-tidy check: Assignment and
>>>>> Construction" to "clang-tidy check: rule-of-five".
>>>>> jbcoe removed rL LLVM as the repository for this revision.
>>>>> jbcoe updated this revision to Diff 46775.
>>>>> jbcoe added a comment.
>>>>>
>>>>> I've responded to review comments (thanks for those) and renamed the
>>>>> check to 'rule-of-five'.
>>>>>
>>>>> I think it needs moving to cppcoreguidelines and needs destructor
>>>>> handling adding to it. As suggested, I'll address that in a later patch if
>>>>> everything else looks ok.
>>>>>
>>>>>
>>>>> http://reviews.llvm.org/D16376
>>>>>
>>>>> Files:
>>>>>   clang-tidy/misc/CMakeLists.txt
>>>>>   clang-tidy/misc/MiscTidyModule.cpp
>>>>>   clang-tidy/misc/RuleOfFiveCheck.cpp
>>>>>   clang-tidy/misc/RuleOfFiveCheck.h
>>>>>   docs/clang-tidy/checks/list.rst
>>>>>   docs/clang-tidy/checks/misc-rule-of-five.rst
>>>>>   test/clang-tidy/misc-rule-of-five.cpp
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160203/091c5825/attachment-0001.html>