This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] add check cppcoreguidelines-special-member-functions
ClosedPublic

Authored by jbcoe on Jul 19 2016, 7:23 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

jbcoe updated this revision to Diff 64495.Jul 19 2016, 7:23 AM
jbcoe retitled this revision from to clang-tidy check: cppcoreguidelines-rule-of-five-and-zero.
jbcoe updated this object.
jbcoe added a reviewer: aaron.ballman.
jbcoe retitled this revision from clang-tidy check: cppcoreguidelines-rule-of-five-and-zero to [clang-tidy] add check cppcoreguidelines-rule-of-five-and-zero.Jul 19 2016, 7:24 AM
jbcoe updated this object.
Eugene.Zelenko set the repository for this revision to rL LLVM.
Eugene.Zelenko added a subscriber: cfe-commits.

Please mention this check in docs/ReleaseNotes.rst. See pre-4.0 branch versions as example.

docs/clang-tidy/checks/cppcoreguidelines-rule-of-five-and-zero.rst
15 ↗(On Diff #64495)

Please add space between = and delete.

test/clang-tidy/cppcoreguidelines-rule-of-five-and-zero.cpp
12 ↗(On Diff #64495)

Please separate different cases with empty lines.

39 ↗(On Diff #64495)

Looks like = delete is missed fall all class methods.

Will be good idea to introduce similar check for C++98/03.

Prazek added a project: Restricted Project.Jul 19 2016, 2:09 PM
Prazek added a subscriber: Prazek.Jul 19 2016, 2:30 PM
Prazek added inline comments.
clang-tidy/cppcoreguidelines/RuleOfFiveAndZeroCheck.cpp
73–77 ↗(On Diff #64495)

I think it would be much better to aggregate the diagnosics.
E.g. if I declare 4 special functions then I will get 4 lines of warnings, and I won't even know what function did I forgot to declare.

So it should be better to fire diag on the first, or just one of the special function and say "class %0 defines {{list_here}} but doesn't define {{other_list}}"

clang-tidy/cppcoreguidelines/RuleOfFiveAndZeroCheck.h
19 ↗(On Diff #64495)

no comma after all. But I might be wrong because I am not certified grammar nazi

docs/clang-tidy/checks/cppcoreguidelines-rule-of-five-and-zero.rst
11 ↗(On Diff #64495)

typo s/expliciti/explicit

aaron.ballman added inline comments.Jul 21 2016, 8:05 AM
clang-tidy/cppcoreguidelines/RuleOfFiveAndZeroCheck.cpp
63 ↗(On Diff #64495)

No newline.

72 ↗(On Diff #64495)

No newline.

73–77 ↗(On Diff #64495)

I tend to agree -- this will get very chatty if I define 4 out of the 5 special member functions, so it might be best if the diagnostic instead reads:

"class %0 defines %1, but does not define %2"

Where %1 is the list of things the class defines and %2 is the list of things the class fails to define.

Note, "define or delete" is a bit weird because a deleted function is a definition, just like an explicitly-defaulted function is a definition. It seems strange to mention delete but not default, so I just went with "define" by itself for brevity.

clang-tidy/cppcoreguidelines/RuleOfFiveAndZeroCheck.h
19 ↗(On Diff #64495)

The comma is correct there. :-)

test/clang-tidy/cppcoreguidelines-rule-of-five-and-zero.cpp
44 ↗(On Diff #64495)

I'd also like to see a test case with mixed =default and =delete (to ensure we do not trigger when explicitly defaulting or deleting the some special member functions).

jbcoe updated this revision to Diff 65092.Jul 22 2016, 9:54 AM
jbcoe removed rL LLVM as the repository for this revision.
jbcoe marked 2 inline comments as done.

Addressed review comments, thanks for those, check is nicer now.

jbcoe updated this revision to Diff 65096.Jul 22 2016, 10:08 AM
jbcoe marked 6 inline comments as done.

Address comments from review, thanks for those.

Since rule name is different in C++98/03 and C++11 or newer it will make sense to create two checks which will work only for their respective versions (of course, implementation should be shared). Or create alias name.

Check is still not mentioned in docs/ReleaseNotes.rst.

I will look again soon, but it looks much better right now!

clang-tidy/cppcoreguidelines/RuleOfFiveAndZeroCheck.cpp
43–57 ↗(On Diff #65096)

maybe make it a DenseMap? (or maybe there is SmallMap or SmallDense map or something similar)

85–105 ↗(On Diff #65096)

This looks like it could be changed into for
for (auto [str, kind] : kinds) {

if( ... Result.Nodes.getNodeAs<FunctionDecl>(str))

I havent tried putting Base type to getNodeAs, but I think it should work.

Since rule name is different in C++98/03 and C++11 or newer it will make sense to create two checks which will work only for their respective versions (of course, implementation should be shared). Or create alias name.

Check is still not mentioned in docs/ReleaseNotes.rst.

This seems like too much work, maybe just cppcoreguidelines-rule-of-3-or-5?

jbcoe updated this revision to Diff 65414.Jul 25 2016, 1:55 PM
jbcoe set the repository for this revision to rL LLVM.
jbcoe marked an inline comment as done.

Rename to cppcoreguidelines-special-member-functions to avoid the duplication required with naming it rule-of-3 and rule-of-5.

Reduce code duplication by iterating over a list of matchers.

Use dense map. [FIXME: I can't get DenseMap to work with the DenseMapInfo defined in the header. IdentifierNamingCheck.cpp does this without any issues but I hit compiler errors around DenseMap and SourceLocation. Moving the specialisation from the header to the source of SpecialMemberFunctionsCheck suffices to repeat the problem. Any input here would be appreciated.]

Eugene.Zelenko added inline comments.Jul 25 2016, 2:02 PM
docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst
4 ↗(On Diff #65414)

Size should be same as size of name above.

jbcoe updated this revision to Diff 65423.Jul 25 2016, 2:33 PM
jbcoe removed rL LLVM as the repository for this revision.

Fix underline length in docs.

jbcoe marked an inline comment as done.Jul 25 2016, 2:34 PM
jbcoe retitled this revision from [clang-tidy] add check cppcoreguidelines-rule-of-five-and-zero to [clang-tidy] add check cppcoreguidelines-special-member-functions.Jul 26 2016, 6:59 AM
aaron.ballman added inline comments.Jul 28 2016, 7:28 AM
clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
62 ↗(On Diff #65423)

I think this join function can maybe be replaced by a call to llvm::join() from StringExtras.h?

64 ↗(On Diff #65423)

It's good to put an && along with some explanatory text for debugging if this assertion ever triggers.

73 ↗(On Diff #65423)

Elide braces per usual style conventions.

94 ↗(On Diff #65423)

The & should bind to KV rather than auto (clang-format should handle this for you).

clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
42–44 ↗(On Diff #65423)

Do these need to be public?

62–63 ↗(On Diff #65423)

Any reason why not to do this as part of this patch?

jbcoe updated this revision to Diff 66011.Jul 28 2016, 2:37 PM

Minor changes from review.

clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
62 ↗(On Diff #65423)

I want to join the last member function with "and" or "for", llvm::join won't let me do that.
Thanks for the suggestion though. I need to explore more of llvm/ADT.

clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
42–44 ↗(On Diff #65423)

I think so, I can't get the specialisation of DenseMapInfo to work otherwise.

62–63 ↗(On Diff #65423)

I've spent days trying. I can't get code to compile if it's moved and can't work out why. As the FIXME says, it works in IdentifierNaming.

aaron.ballman added inline comments.Jul 29 2016, 6:42 AM
clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
60 ↗(On Diff #66011)

Btw, with MSVC, this will give you a "not all control paths return a value" warning. You should put an llvm_unreachable() after the switch to silence that diagnostic.

63 ↗(On Diff #66011)

Ah, crud, good point.

jbcoe updated this revision to Diff 66161.Jul 29 2016, 12:16 PM
jbcoe set the repository for this revision to rL LLVM.

Fix MSVC warning.

jbcoe marked 9 inline comments as done.Jul 29 2016, 12:17 PM
aaron.ballman accepted this revision.Jul 29 2016, 2:56 PM
aaron.ballman edited edge metadata.

LGTM, thank you for the check! The oddness with the DenseMap can wait until a follow-up patch, btw.

This revision is now accepted and ready to land.Jul 29 2016, 2:56 PM
This revision was automatically updated to reflect the committed changes.
alexfh edited edge metadata.Aug 1 2016, 12:47 AM

Thanks for the new check! Looks awesome! A couple of late comments inline.

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
91

Is the explicit use of the initializer_list needed to pacify MSVC? I wonder whether std::string can be replaced with a StringRef?

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp
6

Native speakers can correct me, but "does not define X or Y" reads as if defining either X or Y is enough to fix the issue, while in fact we're expecting all special members to be defined. I'm not sure what the best wording would be, maybe "class 'C' defines X and Y, but does not define all other special members: T, U, V"?

I have an segfault on all the source files of my project when I enable this check (it works ok when I disable this check). Sorry I don't have time to post a minimal source file producing the segfault. I will maybe tomorrow, or in two weeks.