Page MenuHomePhabricator

[clang-tidy] Added options to cppcoreguidelines-special-member-functions check
ClosedPublic

Authored by fgross on Mar 4 2017, 9:23 AM.

Details

Summary

Added two options to cppcoreguidelines-special-member-functions to allow a more relaxed checking.

With 'AllowSoleDefaultDtor' set to 1, classes with explicitly defaulted destructors but no other special members are not flagged anymore:

class foo {
 // typical scenario: defaulted virtual destructor in base classes
  virtual ~foo() = default;
};

With 'AllowMissingMoveFunctions' set to 1, classes with no move functions at all are not flagged anymore:

class bar{
 ~bar();
 bar(const bar&);
 bar& operator=(const bar&);
};

Diff Detail

Event Timeline

fgross created this revision.Mar 4 2017, 9:23 AM
aaron.ballman requested changes to this revision.Mar 4 2017, 12:32 PM
aaron.ballman added a subscriber: aaron.ballman.

Missing documentation changes for the new options.

clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
112

Please qualify the find with std::, here and elsewhere.

140–171

Formatting

142

{ should go up a line.

155

Can elide the parens.

169

This feels like a lot of code duplication.

clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
51

You should run clang-format over the patch; the & should bind to ID.

This revision now requires changes to proceed.Mar 4 2017, 12:32 PM
fgross updated this revision to Diff 90593.Mar 4 2017, 3:21 PM
fgross edited edge metadata.

Updated documentation, got rid of code duplication.

fgross marked 6 inline comments as done.Mar 4 2017, 3:28 PM
fgross added inline comments.
clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
112

It's actually llvm::; added namespace and switched to is_contained.

fgross updated this revision to Diff 90607.Mar 5 2017, 6:26 AM
fgross marked an inline comment as done.

reformatted

aaron.ballman added inline comments.Mar 8 2017, 8:29 PM
clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
116

const auto *Dtor

docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst
23

Examples for each of the options might be nice.

fgross updated this revision to Diff 91183.Mar 9 2017, 10:08 AM

Added examples to options doc.

fgross marked 2 inline comments as done.Mar 9 2017, 10:09 AM
fgross updated this revision to Diff 91188.Mar 9 2017, 10:13 AM

Diff was missing cppcoreguidelines-special-member-functions-relaxed.cpp...

This revision is now accepted and ready to land.Mar 12 2017, 3:30 PM
alexfh added inline comments.Mar 12 2017, 4:11 PM
clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
184

A note on usability: the diagnostic might be more useful, if it had a note pointing to each of the special members mentioned in %1. Just a thought without any relevance to this patch.

No commit access, could someone please take care of this? Thanks!

aaron.ballman closed this revision.Mar 13 2017, 2:51 PM

I've commit in r297671, thanks!

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;
}

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?

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.
If you've written a destructor then you probably need to write copy functions to avoid double free.
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.
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.
If you've deleted the copy functions, then you probably don't want the move functions either, so AllowDeletedCopyFunctions would make sense.

I haven't asked the Core Guidelines folks.

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?

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.

The scenario I had in mind was legacy code which was written back when it still was the "rule of three" instead of the "rule of five". Those classes with defined destructor and copy operations are still perfectly safe, because moving them falls back to copying. They may not be perfectly tuned for performance, but having no move operations is not an indication for some resoure management error. That's why I do think this options makes sense.

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;
}

My 2 cents on this one: Even with AllowMissingMoveFunctions=1 at least the missing destructor definition should be diagnosed, because it violates the classic rule of three. If you delete your copy operations, you likely have some resources that need to be taken care of in your destructor, so this piece of code would worry me. Better be clear about it and explicitly default the destructor.

I have 1000s of warnings from this check.

A lot of them are about google tests:
warning: class 'Foo_Bar_Test' defines a copy constructor and a copy assignment operator but does not define a destructor [cppcoreguidelines-special-member-functions]
I'd like an option to suppress these.

It warns about classes that use boost::noncopyable:
warning: class 'Foo' defines a non-default destructor but does not define a copy constructor or a copy assignment operator [cppcoreguidelines-special-member-functions]
class Foo : boost::noncopyable
I think this is a bug in the check.

My 2 cents on this one: Even with AllowMissingMoveFunctions=1 at least the missing destructor definition should be diagnosed, because it violates the classic rule of three. If you delete your copy operations, you likely have some resources that need to be taken care of in your destructor, so this piece of code would worry me. Better be clear about it and explicitly default the destructor.

This is a rule of zero class, but with copying disabled; the resources are just as safe as with a normal rule of zero class.

I have 1000s of warnings from this check.

A lot of them are about google tests:
warning: class 'Foo_Bar_Test' defines a copy constructor and a copy assignment operator but does not define a destructor [cppcoreguidelines-special-member-functions]
I'd like an option to suppress these.

It warns about classes that use boost::noncopyable:
warning: class 'Foo' defines a non-default destructor but does not define a copy constructor or a copy assignment operator [cppcoreguidelines-special-member-functions]
class Foo : boost::noncopyable
I think this is a bug in the check.

It is not a bug in the check; the check implements the C++ Core Guideline rule. Hence the comment about discussing it with them.

I think your use cases are compelling and might warrant an option. However, with that in mind combined with AllowMissingMoveFunctions, it makes me think this check should not have any of the options -- it's a sign that your code base isn't ready to comply with C.21 in C++ Core Guidelines. The wording for the guideline is pretty specific about what the authors intend: "Compilers enforce much of this rule and ideally warn about any violation." and its enforcement "(Simple) A class should have a declaration (even a =delete one) for either all or none of the special functions." I'm not saying we shouldn't add this option you're looking for, but I'm still a bit uncomfortable with coming up with option combinations that effectively disable the entire point to a check in a coding standard -- that's why we allow you to disable the check more visibly.

My 2 cents on this one: Even with AllowMissingMoveFunctions=1 at least the missing destructor definition should be diagnosed, because it violates the classic rule of three. If you delete your copy operations, you likely have some resources that need to be taken care of in your destructor, so this piece of code would worry me. Better be clear about it and explicitly default the destructor.

This is a rule of zero class, but with copying disabled; the resources are just as safe as with a normal rule of zero class.

Depending on the resource; you might allocate a resource in the constructor, deleted copy (and thus no move) operators, but still want to release the resource in the destructor.