[PATCH] D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check
aaron.ballman added a comment. In https://reviews.llvm.org/D30610#934891, @malcolm.parsons wrote: > 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. > In https://reviews.llvm.org/D30610#934862, @fgross wrote: > >> 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. https://reviews.llvm.org/D30610 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check
malcolm.parsons added a comment. 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. In https://reviews.llvm.org/D30610#934862, @fgross wrote: > 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. https://reviews.llvm.org/D30610 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check
fgross added a comment.
> 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.
https://reviews.llvm.org/D30610
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check
aaron.ballman added a comment.
In https://reviews.llvm.org/D30610#934704, @malcolm.parsons wrote:
> In https://reviews.llvm.org/D30610#934617, @aaron.ballman wrote:
>
> > In https://reviews.llvm.org/D30610#934452, @malcolm.parsons wrote:
> >
> > > 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?
https://reviews.llvm.org/D30610
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check
malcolm.parsons added a comment.
In https://reviews.llvm.org/D30610#934617, @aaron.ballman wrote:
> In https://reviews.llvm.org/D30610#934452, @malcolm.parsons wrote:
>
> > 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.
https://reviews.llvm.org/D30610
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check
aaron.ballman added a comment.
In https://reviews.llvm.org/D30610#934452, @malcolm.parsons wrote:
> 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?
https://reviews.llvm.org/D30610
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check
malcolm.parsons added a comment.
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;
}
https://reviews.llvm.org/D30610
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check
aaron.ballman closed this revision. aaron.ballman added a comment. I've commit in r297671, thanks! https://reviews.llvm.org/D30610 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check
fgross added a comment. No commit access, could someone please take care of this? Thanks! https://reviews.llvm.org/D30610 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check
alexfh added inline comments. Comment at: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp:184 + if (!MissingMembers.empty()) +diag(ID.first, "class '%0' defines %1 but does not define %2") +<< ID.second << join(DefinedMembers, " and ") 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. https://reviews.llvm.org/D30610 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D30610 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check
fgross updated this revision to Diff 91188.
fgross added a comment.
Diff was missing cppcoreguidelines-special-member-functions-relaxed.cpp...
https://reviews.llvm.org/D30610
Files:
clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst
test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp
test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp
test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
Index: test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
===
--- test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
+++ test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
@@ -3,7 +3,12 @@
class DefinesDestructor {
~DefinesDestructor();
};
-// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
+
+class DefinesDefaultedDestructor {
+ ~DefinesDefaultedDestructor() = default;
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDefaultedDestructor' defines a default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
class DefinesCopyConstructor {
DefinesCopyConstructor(const DefinesCopyConstructor &);
Index: test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp
===
--- test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp
+++ test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp
@@ -0,0 +1,71 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-special-member-functions %t -- -config="{CheckOptions: [{key: cppcoreguidelines-special-member-functions.AllowMissingMoveFunctions, value: 1}, {key: cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor, value: 1}]}" --
+
+class DefinesDestructor {
+ ~DefinesDestructor();
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a non-default destructor but does not define a copy constructor or a copy assignment operator [cppcoreguidelines-special-member-functions]
+
+class DefinesDefaultedDestructor {
+ ~DefinesDefaultedDestructor() = default;
+};
+
+class DefinesCopyConstructor {
+ DefinesCopyConstructor(const DefinesCopyConstructor &);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesCopyConstructor' defines a copy constructor but does not define a destructor or a copy assignment operator [cppcoreguidelines-special-member-functions]
+
+class DefinesCopyAssignment {
+ DefinesCopyAssignment =(const DefinesCopyAssignment &);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesCopyAssignment' defines a copy assignment operator but does not define a destructor or a copy constructor [cppcoreguidelines-special-member-functions]
+
+class DefinesMoveConstructor {
+ DefinesMoveConstructor(DefinesMoveConstructor &&);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesMoveConstructor' defines a move constructor but does not define a destructor, a copy constructor, a copy assignment operator or a move assignment operator [cppcoreguidelines-special-member-functions]
+
+class DefinesMoveAssignment {
+ DefinesMoveAssignment =(DefinesMoveAssignment &&);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesMoveAssignment' defines a move assignment operator but does not define a destructor, a copy constructor, a copy assignment operator or a move constructor [cppcoreguidelines-special-member-functions]
+class DefinesNothing {
+};
+
+class DefinesEverything {
+ DefinesEverything(const DefinesEverything &);
+ DefinesEverything =(const DefinesEverything &);
+ DefinesEverything(DefinesEverything &&);
+ DefinesEverything =(DefinesEverything &&);
+ ~DefinesEverything();
+};
+
+class DeletesEverything {
+ DeletesEverything(const DeletesEverything &) = delete;
+ DeletesEverything =(const DeletesEverything &) = delete;
+ DeletesEverything(DeletesEverything &&) = delete;
+ DeletesEverything =(DeletesEverything &&) = delete;
+ ~DeletesEverything() = delete;
+};
+
+class DeletesCopyDefaultsMove {
+ DeletesCopyDefaultsMove(const DeletesCopyDefaultsMove &) = delete;
+ DeletesCopyDefaultsMove =(const DeletesCopyDefaultsMove &) = delete;
+ DeletesCopyDefaultsMove(DeletesCopyDefaultsMove &&) = default;
+
[PATCH] D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check
fgross updated this revision to Diff 91183.
fgross added a comment.
Added examples to options doc.
https://reviews.llvm.org/D30610
Files:
clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst
test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp
test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
Index: test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
===
--- test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
+++ test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
@@ -3,7 +3,12 @@
class DefinesDestructor {
~DefinesDestructor();
};
-// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
+
+class DefinesDefaultedDestructor {
+ ~DefinesDefaultedDestructor() = default;
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDefaultedDestructor' defines a default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
class DefinesCopyConstructor {
DefinesCopyConstructor(const DefinesCopyConstructor &);
Index: test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp
===
--- test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp
+++ test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp
@@ -3,7 +3,7 @@
class DefinesDestructor {
~DefinesDestructor();
};
-// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a destructor but does not define a copy constructor or a copy assignment operator [cppcoreguidelines-special-member-functions]
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a non-default destructor but does not define a copy constructor or a copy assignment operator [cppcoreguidelines-special-member-functions]
class DefinesCopyConstructor {
DefinesCopyConstructor(const DefinesCopyConstructor &);
Index: docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst
===
--- docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst
+++ docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst
@@ -19,3 +19,31 @@
Guidelines, corresponding to rule C.21. See
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c21-if-you-define-or-delete-any-default-operation-define-or-delete-them-all.
+
+Options
+---
+
+.. option:: AllowSoleDefaultDtor
+
+ When set to `1` (default is `0`), this check doesn't flag classes with a sole, explicitly
+ defaulted destructor. An example for such a class is:
+
+ .. code-block:: c++
+
+ struct A {
+ virtual ~A() = default;
+ };
+
+.. option:: AllowMissingMoveFunctions
+
+ When set to `1` (default is `0`), this check doesn't flag classes which define no move
+ operations at all. It still flags classes which define only one of either
+ move constructor or move assignment operator. With this option enabled, the following class won't be flagged:
+
+ .. code-block:: c++
+
+ struct A {
+ A(const A&);
+ A& operator=(const A&);
+ ~A();
+ }
Index: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
===
--- clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
+++ clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
@@ -25,14 +25,16 @@
/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-special-member-functions.html
class SpecialMemberFunctionsCheck : public ClangTidyCheck {
public:
- SpecialMemberFunctionsCheck(StringRef Name, ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context) {}
+ SpecialMemberFunctionsCheck(StringRef Name, ClangTidyContext *Context);
+ void storeOptions(ClangTidyOptions::OptionMap ) override;
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult ) override;
void onEndOfTranslationUnit() override;
enum class SpecialMemberFunctionKind : uint8_t {
Destructor,
+DefaultDestructor,
+NonDefaultDestructor,
CopyConstructor,
CopyAssignment,
[PATCH] D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check
aaron.ballman added inline comments.
Comment at: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp:116
+
+ if (auto Dtor = Result.Nodes.getNodeAs("dtor")) {
+StoreMember(Dtor->isDefaulted()
`const auto *Dtor`
Comment at:
docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst:23
+
+Options
+---
Examples for each of the options might be nice.
https://reviews.llvm.org/D30610
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check
fgross updated this revision to Diff 90607.
fgross marked an inline comment as done.
fgross added a comment.
reformatted
https://reviews.llvm.org/D30610
Files:
clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst
test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp
test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp
test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
Index: test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
===
--- test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
+++ test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
@@ -3,7 +3,12 @@
class DefinesDestructor {
~DefinesDestructor();
};
-// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
+
+class DefinesDefaultedDestructor {
+ ~DefinesDefaultedDestructor() = default;
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDefaultedDestructor' defines a default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
class DefinesCopyConstructor {
DefinesCopyConstructor(const DefinesCopyConstructor &);
Index: test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp
===
--- test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp
+++ test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp
@@ -0,0 +1,71 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-special-member-functions %t -- -config="{CheckOptions: [{key: cppcoreguidelines-special-member-functions.AllowMissingMoveFunctions, value: 1}, {key: cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor, value: 1}]}" --
+
+class DefinesDestructor {
+ ~DefinesDestructor();
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a non-default destructor but does not define a copy constructor or a copy assignment operator [cppcoreguidelines-special-member-functions]
+
+class DefinesDefaultedDestructor {
+ ~DefinesDefaultedDestructor() = default;
+};
+
+class DefinesCopyConstructor {
+ DefinesCopyConstructor(const DefinesCopyConstructor &);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesCopyConstructor' defines a copy constructor but does not define a destructor or a copy assignment operator [cppcoreguidelines-special-member-functions]
+
+class DefinesCopyAssignment {
+ DefinesCopyAssignment =(const DefinesCopyAssignment &);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesCopyAssignment' defines a copy assignment operator but does not define a destructor or a copy constructor [cppcoreguidelines-special-member-functions]
+
+class DefinesMoveConstructor {
+ DefinesMoveConstructor(DefinesMoveConstructor &&);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesMoveConstructor' defines a move constructor but does not define a destructor, a copy constructor, a copy assignment operator or a move assignment operator [cppcoreguidelines-special-member-functions]
+
+class DefinesMoveAssignment {
+ DefinesMoveAssignment =(DefinesMoveAssignment &&);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesMoveAssignment' defines a move assignment operator but does not define a destructor, a copy constructor, a copy assignment operator or a move constructor [cppcoreguidelines-special-member-functions]
+class DefinesNothing {
+};
+
+class DefinesEverything {
+ DefinesEverything(const DefinesEverything &);
+ DefinesEverything =(const DefinesEverything &);
+ DefinesEverything(DefinesEverything &&);
+ DefinesEverything =(DefinesEverything &&);
+ ~DefinesEverything();
+};
+
+class DeletesEverything {
+ DeletesEverything(const DeletesEverything &) = delete;
+ DeletesEverything =(const DeletesEverything &) = delete;
+ DeletesEverything(DeletesEverything &&) = delete;
+ DeletesEverything =(DeletesEverything &&) = delete;
+ ~DeletesEverything() = delete;
+};
+
+class DeletesCopyDefaultsMove {
+ DeletesCopyDefaultsMove(const DeletesCopyDefaultsMove &) = delete;
+ DeletesCopyDefaultsMove =(const DeletesCopyDefaultsMove &) = delete;
+ DeletesCopyDefaultsMove(DeletesCopyDefaultsMove &&) = default;
+ DeletesCopyDefaultsMove
[PATCH] D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check
fgross marked 6 inline comments as done. fgross added inline comments. Comment at: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp:110 +ClassWithSpecialMembers[ID]; +if (find(Members, Kind) == Members.end()) + Members.push_back(Kind); aaron.ballman wrote: > Please qualify the find with std::, here and elsewhere. It's actually llvm::; added namespace and switched to is_contained. https://reviews.llvm.org/D30610 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check
fgross updated this revision to Diff 90593.
fgross added a comment.
Updated documentation, got rid of code duplication.
https://reviews.llvm.org/D30610
Files:
clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst
test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp
test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp
test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
Index: test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
===
--- test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
+++ test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
@@ -3,7 +3,12 @@
class DefinesDestructor {
~DefinesDestructor();
};
-// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
+
+class DefinesDefaultedDestructor {
+ ~DefinesDefaultedDestructor() = default;
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDefaultedDestructor' defines a default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
class DefinesCopyConstructor {
DefinesCopyConstructor(const DefinesCopyConstructor &);
Index: test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp
===
--- test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp
+++ test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp
@@ -0,0 +1,71 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-special-member-functions %t -- -config="{CheckOptions: [{key: cppcoreguidelines-special-member-functions.AllowMissingMoveFunctions, value: 1}, {key: cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor, value: 1}]}" --
+
+class DefinesDestructor {
+ ~DefinesDestructor();
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a non-default destructor but does not define a copy constructor or a copy assignment operator [cppcoreguidelines-special-member-functions]
+
+class DefinesDefaultedDestructor {
+ ~DefinesDefaultedDestructor() = default;
+};
+
+class DefinesCopyConstructor {
+ DefinesCopyConstructor(const DefinesCopyConstructor &);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesCopyConstructor' defines a copy constructor but does not define a destructor or a copy assignment operator [cppcoreguidelines-special-member-functions]
+
+class DefinesCopyAssignment {
+ DefinesCopyAssignment =(const DefinesCopyAssignment &);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesCopyAssignment' defines a copy assignment operator but does not define a destructor or a copy constructor [cppcoreguidelines-special-member-functions]
+
+class DefinesMoveConstructor {
+ DefinesMoveConstructor(DefinesMoveConstructor &&);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesMoveConstructor' defines a move constructor but does not define a destructor, a copy constructor, a copy assignment operator or a move assignment operator [cppcoreguidelines-special-member-functions]
+
+class DefinesMoveAssignment {
+ DefinesMoveAssignment =(DefinesMoveAssignment &&);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesMoveAssignment' defines a move assignment operator but does not define a destructor, a copy constructor, a copy assignment operator or a move constructor [cppcoreguidelines-special-member-functions]
+class DefinesNothing {
+};
+
+class DefinesEverything {
+ DefinesEverything(const DefinesEverything &);
+ DefinesEverything =(const DefinesEverything &);
+ DefinesEverything(DefinesEverything &&);
+ DefinesEverything =(DefinesEverything &&);
+ ~DefinesEverything();
+};
+
+class DeletesEverything {
+ DeletesEverything(const DeletesEverything &) = delete;
+ DeletesEverything =(const DeletesEverything &) = delete;
+ DeletesEverything(DeletesEverything &&) = delete;
+ DeletesEverything =(DeletesEverything &&) = delete;
+ ~DeletesEverything() = delete;
+};
+
+class DeletesCopyDefaultsMove {
+ DeletesCopyDefaultsMove(const DeletesCopyDefaultsMove &) = delete;
+ DeletesCopyDefaultsMove =(const DeletesCopyDefaultsMove &) = delete;
+ DeletesCopyDefaultsMove(DeletesCopyDefaultsMove &&) = default;
+ DeletesCopyDefaultsMove
[PATCH] D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check
aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.
Missing documentation changes for the new options.
Comment at: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp:110
+ClassWithSpecialMembers[ID];
+if (find(Members, Kind) == Members.end())
+ Members.push_back(Kind);
Please qualify the find with std::, here and elsewhere.
Comment at: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp:138
- for (const auto : ClassWithSpecialMembers) {
-const auto = C.second;
+void SpecialMemberFunctionsCheck::checkForMissingMembers(const ClassDefId& ID,
+llvm::ArrayRef DefinedSpecialMembers)
Formatting
Comment at: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp:140
+llvm::ArrayRef DefinedSpecialMembers)
+{
+ llvm::SmallVector MissingSpecialMembers;
{ should go up a line.
Comment at: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp:153
+
+ auto RequireDtor = [&]() {
+if (!HasMember(SpecialMemberFunctionKind::DefaultDestructor) &&
Can elide the parens.
Comment at: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp:167
+RequireMember(SpecialMemberFunctionKind::CopyAssignment);
+if (!AllowMissingMoveFunctions && getLangOpts().CPlusPlus11) {
+ RequireMember(SpecialMemberFunctionKind::MoveConstructor);
This feels like a lot of code duplication.
Comment at: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h:51
private:
+ void checkForMissingMembers(const ClassDefId& ID,
+llvm::ArrayRef DefinedSpecialMembers);
You should run clang-format over the patch; the & should bind to ID.
https://reviews.llvm.org/D30610
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check
fgross created this revision.
Herald added subscribers: JDevlieghere, nemanjai.
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&);
};
https://reviews.llvm.org/D30610
Files:
clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp
test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp
test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
Index: test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
===
--- test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
+++ test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
@@ -3,7 +3,12 @@
class DefinesDestructor {
~DefinesDestructor();
};
-// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
+
+class DefinesDefaultedDestructor {
+ ~DefinesDefaultedDestructor() = default;
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDefaultedDestructor' defines a default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
class DefinesCopyConstructor {
DefinesCopyConstructor(const DefinesCopyConstructor &);
Index: test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp
===
--- test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp
+++ test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp
@@ -0,0 +1,71 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-special-member-functions %t -- -config="{CheckOptions: [{key: cppcoreguidelines-special-member-functions.AllowMissingMoveFunctions, value: 1}, {key: cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor, value: 1}]}" --
+
+class DefinesDestructor {
+ ~DefinesDestructor();
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a non-default destructor but does not define a copy constructor or a copy assignment operator [cppcoreguidelines-special-member-functions]
+
+class DefinesDefaultedDestructor {
+ ~DefinesDefaultedDestructor() = default;
+};
+
+class DefinesCopyConstructor {
+ DefinesCopyConstructor(const DefinesCopyConstructor &);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesCopyConstructor' defines a copy constructor but does not define a destructor or a copy assignment operator [cppcoreguidelines-special-member-functions]
+
+class DefinesCopyAssignment {
+ DefinesCopyAssignment =(const DefinesCopyAssignment &);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesCopyAssignment' defines a copy assignment operator but does not define a destructor or a copy constructor [cppcoreguidelines-special-member-functions]
+
+class DefinesMoveConstructor {
+ DefinesMoveConstructor(DefinesMoveConstructor &&);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesMoveConstructor' defines a move constructor but does not define a destructor, a copy constructor, a copy assignment operator or a move assignment operator [cppcoreguidelines-special-member-functions]
+
+class DefinesMoveAssignment {
+ DefinesMoveAssignment =(DefinesMoveAssignment &&);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesMoveAssignment' defines a move assignment operator but does not define a destructor, a copy constructor, a copy assignment operator or a move constructor [cppcoreguidelines-special-member-functions]
+class DefinesNothing {
+};
+
+class DefinesEverything {
+ DefinesEverything(const DefinesEverything &);
+ DefinesEverything =(const DefinesEverything &);
+ DefinesEverything(DefinesEverything &&);
+ DefinesEverything =(DefinesEverything &&);
+ ~DefinesEverything();
+};
+
+class DeletesEverything {
+ DeletesEverything(const DeletesEverything &) = delete;
+ DeletesEverything =(const DeletesEverything &) =
