This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Check for rule of five and zero.
Needs ReviewPublic

Authored by vrnithinkumar on May 24 2020, 4:05 AM.

Details

Summary

New check to check if a class defines all special members of none of them. This also known as rule of five and zero.
Specified in CppCoreGuidelines: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c21-if-you-define-or-delete-any-default-operation-define-or-delete-them-all.
In summary the check will

  • Checks class defines all special members of none of them.
  • It also see if the base class deletes the special members or not.
  • Has two modes with the flag strict-check.
  • Strict mode will check all or nothing strictly.

For some combination, compiler explicitly delete the special members or does not declare implicitly. In that case don'nt have to define all the special members.
For example, in case of defining all members except move constructor and move assignment compiler will not implicitly declare the move constructor and move assignment.
For non strict mode we will consider these combinations as safe.

I found one review https://reviews.llvm.org/D16376 already related to this.
I modified my changes based on this. But I could not find out why this got abandoned.

Diff Detail

Event Timeline

vrnithinkumar created this revision.May 24 2020, 4:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2020, 4:05 AM

fixed the clang-tidy warnig

lebedev.ri retitled this revision from Check for rule of five and zero. to [clang-tidy] Check for rule of five and zero..May 25 2020, 3:05 PM
jbcoe added a comment.Jun 1 2020, 8:55 AM

https://reviews.llvm.org/D16376 was written by me, I'm afraid I don't recall why I abandoned it and was surprised to see that it had not been submitted.

I don't feel comfortable approving work based on my own but this patch looks great to me.

jbcoe resigned from this revision.Jun 1 2020, 8:55 AM
jbcoe added a subscriber: jbcoe.

Perhaps associated is https://github.com/isocpp/CppCoreGuidelines/issues/870 which was closed but never (in my mind) resolved.

It would take more complex matching and diagnostics logic, but it would be so much nicer if the diagnostics were grouped. So instead of having a diagnostic about class X defining a copy construct and another about it defining a copy assignment operator, It could just say X defines a copy constructor and copy assignment operator without all 5 special members defined. The optionally matcher could be handy in implementing this.

clang-tools-extra/clang-tidy/cppcoreguidelines/RuleOfFiveAndZeroCheck.cpp
23

You need to implement the storeOptions method to store this.

26–27

move this to the isLanguageVersionSupported virtual function

82

You don't need to specify allOf here, top level matchers are implicitly allOf matchers. Same goes for all cases below.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-rule-of-five-and-zero-strict.cpp
19–23

Probably doesn't affect the tests, but shouldn't these all be = delete;?

MTC added a subscriber: MTC.Aug 16 2021, 7:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2021, 7:17 PM