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
Index: test/clang-tidy/misc-rule-of-five.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/misc-rule-of-five.cpp
@@ -0,0 +1,209 @@
+// RUN: %check_clang_tidy %s misc-rule-of-five %t
+
+//
+// User defined copy constructors
+//
+class A {
+ A(const A &);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: class 'A' defines a copy constructor but not a copy assignment operator [misc-rule-of-five]
+};
+
+// CHECK-FIXES: class A {
+// CHECK-FIXES-NEXT: A(const A &);
+// CHECK-FIXES-NEXT: A &operator=(const A &) = delete;
+// CHECK-FIXES-NEXT: //
+// CHECK-FIXES-NEXT: };
+
+class B {
+ B(const B &);
+ B &operator=(const B &);
+};
+
+class C {
+ C(const C &) = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: class 'C' defines a copy constructor but not a copy assignment operator [misc-rule-of-five]
+};
+
+// CHECK-FIXES: class C {
+// CHECK-FIXES-NEXT: C(const C &) = default;
+// CHECK-FIXES-NEXT: C &operator=(const C &) = default;
+// CHECK-FIXES-NEXT: //
+// CHECK-FIXES-NEXT: };
+
+class D {
+ D(const D &);
+ D &operator=(const D &) = default;
+};
+
+class E {
+ E(const E &);
+ E &operator=(const E &) = delete;
+};
+
+class F {
+ F(const F &) = delete;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: class 'F' defines a copy constructor but not a copy assignment operator [misc-rule-of-five]
+};
+
+// CHECK-FIXES: class F {
+// CHECK-FIXES-NEXT: F(const F &) = delete;
+// CHECK-FIXES-NEXT: F &operator=(const F &) = delete;
+// CHECK-FIXES-NEXT: //
+// CHECK-FIXES-NEXT: };
+
+
+//
+// User defined copy assignment
+//
+class A2 {
+ A2 &operator=(const A2 &);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: class 'A2' defines a copy assignment operator but not a copy constructor [misc-rule-of-five]
+};
+
+// CHECK-FIXES: class A2 {
+// CHECK-FIXES-NEXT: A2(const A2 &) = delete;
+// CHECK-FIXES-NEXT: A2 &operator=(const A2 &);
+// CHECK-FIXES-NEXT: //
+// CHECK-FIXES-NEXT: };
+
+class B2 {
+ B2(const B2 &);
+ B2 &operator=(const B2 &);
+};
+
+class C2 {
+ C2 &operator=(const C2 &) = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: class 'C2' defines a copy assignment operator but not a copy constructor [misc-rule-of-five]
+};
+
+// CHECK-FIXES: class C2 {
+// CHECK-FIXES-NEXT: C2(const C2 &) = default;
+// CHECK-FIXES-NEXT: C2 &operator=(const C2 &) = default;
+// CHECK-FIXES-NEXT: //
+// CHECK-FIXES-NEXT: };
+
+class D2 {
+ D2(const D2 &) = default;
+ D2 &operator=(const D2 &);
+};
+
+class E2 {
+ E2(const E2 &) = delete;
+ E2 &operator=(const E2 &);
+};
+
+class F2 {
+ F2 &operator=(const F2 &) = delete;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: class 'F2' defines a copy assignment operator but not a copy constructor [misc-rule-of-five]
+};
+
+// CHECK-FIXES: class F2 {
+// CHECK-FIXES-NEXT: F2(const F2 &) = delete;
+// CHECK-FIXES-NEXT: F2 &operator=(const F2 &) = delete;
+// CHECK-FIXES-NEXT: //
+// CHECK-FIXES-NEXT: };
+
+
+//
+// User defined move constructors
+//
+class A3 {
+ A3(A3 &&);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: class 'A3' defines a move constructor but not a move assignment operator [misc-rule-of-five]
+};
+
+// CHECK-FIXES: class A3 {
+// CHECK-FIXES-NEXT: A3(A3 &&);
+// CHECK-FIXES-NEXT: A3 &operator=(A3 &&) = delete;
+// CHECK-FIXES-NEXT: //
+// CHECK-FIXES-NEXT: };
+
+class B3 {
+ B3(B3 &&);
+ B3 &operator=(B3 &&);
+};
+
+class C3 {
+ C3(C3 &&) = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: class 'C3' defines a move constructor but not a move assignment operator [misc-rule-of-five]
+};
+
+// CHECK-FIXES: class C3 {
+// CHECK-FIXES-NEXT: C3(C3 &&) = default;
+// CHECK-FIXES-NEXT: C3 &operator=(C3 &&) = default;
+// CHECK-FIXES-NEXT: //
+// CHECK-FIXES-NEXT: };
+
+class D3 {
+ D3(D3 &&);
+ D3 &operator=(D3 &&) = default;
+};
+
+class E3 {
+ E3(E3 &&);
+ E3 &operator=(E3 &&) = delete;
+};
+
+class F3 {
+ F3(F3 &&) = delete;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: class 'F3' defines a move constructor but not a move assignment operator [misc-rule-of-five]
+};
+
+// CHECK-FIXES: class F3 {
+// CHECK-FIXES-NEXT: F3(F3 &&) = delete;
+// CHECK-FIXES-NEXT: F3 &operator=(F3 &&) = delete;
+// CHECK-FIXES-NEXT: //
+// CHECK-FIXES-NEXT: };
+
+
+//
+// User defined move assignment
+//
+class A4 {
+ A4 &operator=(A4 &&);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: class 'A4' defines a move assignment operator but not a move constructor [misc-rule-of-five]
+};
+
+// CHECK-FIXES: class A4 {
+// CHECK-FIXES-NEXT: A4(A4 &&) = delete;
+// CHECK-FIXES-NEXT: A4 &operator=(A4 &&);
+// CHECK-FIXES-NEXT: //
+// CHECK-FIXES-NEXT: };
+
+class B4 {
+ B4(B4 &&);
+ B4 &operator=(B4 &&);
+};
+
+class C4 {
+ C4 &operator=(C4 &&) = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: class 'C4' defines a move assignment operator but not a move constructor [misc-rule-of-five]
+};
+
+// CHECK-FIXES: class C4 {
+// CHECK-FIXES-NEXT: C4(C4 &&) = default;
+// CHECK-FIXES-NEXT: C4 &operator=(C4 &&) = default;
+// CHECK-FIXES-NEXT: //
+// CHECK-FIXES-NEXT: };
+
+class D4 {
+ D4(D4 &&) = default;
+ D4 &operator=(D4 &&);
+};
+
+class E4 {
+ E4(E4 &&) = delete;
+ E4 &operator=(E4 &&);
+};
+
+class F4 {
+ F4 &operator=(F4 &&) = delete;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: class 'F4' defines a move assignment operator but not a move constructor [misc-rule-of-five]
+};
+
+// CHECK-FIXES: class F4 {
+// CHECK-FIXES-NEXT: F4(F4 &&) = delete;
+// CHECK-FIXES-NEXT: F4 &operator=(F4 &&) = delete;
+// CHECK-FIXES-NEXT: //
+// CHECK-FIXES-NEXT: };
+
Index: docs/clang-tidy/checks/misc-rule-of-five.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/misc-rule-of-five.rst
@@ -0,0 +1,22 @@
+.. title:: clang-tidy - misc-rule-of-five
+
+misc-rule-of-five
+=========================================
+
+Compilers will generate an assignment operator even if the user defines a copy
+constructor. This behaviour is deprecated by the standard (C++14 standard
+[class.copy] paragraph 18).
+
+"If the class definition does not explicitly declare a copy assignment
+operator, one is declared implicitly. If the class definition declares a move
+constructor or move assignment operator, the implicitly declared copy
+assignment operator 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 constructor or a user-declared destructor."
+
+This check finds classes where only one of the copy/move constructor and
+copy/move assignment operator is user-defined. The fix is defensive: it will
+delete the missing function unless the user has explicitly defaulted the
+defined function. Where the user has defaulted one of the pair, the other will
+be explictly defaulted.
+
Index: docs/clang-tidy/checks/list.rst
===================================================================
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -45,6 +45,7 @@
misc-argument-comment
misc-assert-side-effect
misc-assign-operator-signature
+ misc-rule-of-five
misc-bool-pointer-implicit-conversion
misc-definitions-in-headers
misc-inaccurate-erase
Index: clang-tidy/misc/RuleOfFiveCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/misc/RuleOfFiveCheck.h
@@ -0,0 +1,60 @@
+//===--- RuleOfFiveCheck.h - clang-tidy----*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_RULE_OF_FIVE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_RULE_OF_FIVE_H
+
+#include "../ClangTidy.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Finds copy/move constructors without a matching copy/move assignment
+/// operator
+/// or copy/move assignment operators without a matching copy/move constructor.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-rule-of-five.html
+class RuleOfFiveCheck : public ClangTidyCheck {
+public:
+ RuleOfFiveCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+ enum class SpecialFunctionKind {
+ CopyConstructor,
+ CopyAssignment,
+ MoveConstructor,
+ MoveAssignment
+ };
+
+ static std::string
+ buildFixIt(const CXXMethodDecl &MatchedDecl, StringRef ClassName,
+ RuleOfFiveCheck::SpecialFunctionKind S);
+ static StringRef
+ getDiagnosticFormat(RuleOfFiveCheck::SpecialFunctionKind S);
+ static SourceLocation
+ getInsertionLocation(const CXXMethodDecl &MatchedDecl,
+ const ast_matchers::MatchFinder::MatchResult &Result,
+ RuleOfFiveCheck::SpecialFunctionKind S);
+ void
+ addDiagnosticAndFixIt(const ast_matchers::MatchFinder::MatchResult &Result,
+ const CXXMethodDecl &MatchedDecl,
+ SpecialFunctionKind S);
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_RULE_OF_FIVE_H
Index: clang-tidy/misc/RuleOfFiveCheck.cpp
===================================================================
--- /dev/null
+++ clang-tidy/misc/RuleOfFiveCheck.cpp
@@ -0,0 +1,169 @@
+//===--- RuleOfFiveCheck.cpp - clang-tidy----------------------------------===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "RuleOfFiveCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+void RuleOfFiveCheck::registerMatchers(MatchFinder *Finder) {
+
+ if (!getLangOpts().CPlusPlus)
+ return;
+
+ Finder->addMatcher(
+ cxxRecordDecl(
+ isDefinition(), hasDescendant(cxxConstructorDecl(isCopyConstructor(),
+ unless(isImplicit()))
+ .bind("copy-ctor")),
+ unless(hasDescendant(cxxMethodDecl(isCopyAssignmentOperator())))),
+ this);
+
+ Finder->addMatcher(
+ cxxRecordDecl(
+ isDefinition(),
+ hasDescendant(
+ cxxMethodDecl(isCopyAssignmentOperator(), unless(isImplicit()))
+ .bind("copy assignment")),
+ unless(hasDescendant(cxxConstructorDecl(isCopyConstructor())))),
+ this);
+
+ if (!getLangOpts().CPlusPlus11)
+ return;
+
+ Finder->addMatcher(
+ cxxRecordDecl(
+ isDefinition(), hasDescendant(cxxConstructorDecl(isMoveConstructor(),
+ unless(isImplicit()))
+ .bind("move-ctor")),
+ unless(hasDescendant(cxxMethodDecl(isMoveAssignmentOperator())))),
+ this);
+
+ Finder->addMatcher(
+ cxxRecordDecl(
+ isDefinition(),
+ hasDescendant(
+ cxxMethodDecl(isMoveAssignmentOperator(), unless(isImplicit()))
+ .bind("move assignment")),
+ unless(hasDescendant(cxxConstructorDecl(isMoveConstructor())))),
+ this);
+}
+
+StringRef deleteOrDefault(const CXXMethodDecl &MethodDecl) {
+ return MethodDecl.isDefaulted() ? "default" : "delete";
+}
+
+SourceLocation RuleOfFiveCheck::getInsertionLocation(
+ const CXXMethodDecl &MatchedDecl, const MatchFinder::MatchResult &Result,
+ RuleOfFiveCheck::SpecialFunctionKind S) {
+ switch (S) {
+ case RuleOfFiveCheck::SpecialFunctionKind::CopyConstructor:
+ case RuleOfFiveCheck::SpecialFunctionKind::MoveConstructor: {
+ SourceLocation DeclEnd = Lexer::getLocForEndOfToken(
+ MatchedDecl.getLocEnd(), 0, *Result.SourceManager,
+ Result.Context->getLangOpts());
+ return DeclEnd.getLocWithOffset(1);
+ }
+ case RuleOfFiveCheck::SpecialFunctionKind::CopyAssignment:
+ case RuleOfFiveCheck::SpecialFunctionKind::MoveAssignment: {
+ SourceLocation DeclBegin = MatchedDecl.getLocStart();
+ return DeclBegin.getLocWithOffset(-1);
+ }
+ }
+}
+
+StringRef RuleOfFiveCheck::getDiagnosticFormat(
+ RuleOfFiveCheck::SpecialFunctionKind S) {
+ switch (S) {
+ case RuleOfFiveCheck::SpecialFunctionKind::CopyConstructor:
+ return "class %0 defines a copy constructor but not a copy assignment "
+ "operator";
+ case RuleOfFiveCheck::SpecialFunctionKind::CopyAssignment:
+ return "class %0 defines a copy assignment operator but not a "
+ "copy constructor";
+ case RuleOfFiveCheck::SpecialFunctionKind::MoveConstructor:
+ return "class %0 defines a move constructor but not a move assignment "
+ "operator";
+ case RuleOfFiveCheck::SpecialFunctionKind::MoveAssignment:
+ return "class %0 defines a move assignment operator but not a "
+ "move constructor";
+ }
+};
+
+std::string RuleOfFiveCheck::buildFixIt(
+ const CXXMethodDecl &MatchedDecl, StringRef ClassName,
+ RuleOfFiveCheck::SpecialFunctionKind S) {
+ switch (S) {
+ case RuleOfFiveCheck::SpecialFunctionKind::CopyConstructor:
+ return (llvm::Twine("\n") + ClassName + " &operator=(const " + ClassName +
+ " &) = " + deleteOrDefault(MatchedDecl) + ";")
+ .str();
+ case RuleOfFiveCheck::SpecialFunctionKind::CopyAssignment:
+ return (llvm::Twine("") + ClassName + "(const " + ClassName + " &) = " +
+ deleteOrDefault(MatchedDecl) + ";\n")
+ .str();
+ case RuleOfFiveCheck::SpecialFunctionKind::MoveConstructor:
+ return (llvm::Twine("\n") + ClassName + " &operator=(" + ClassName +
+ " &&) = " + deleteOrDefault(MatchedDecl) + ";")
+ .str();
+ case RuleOfFiveCheck::SpecialFunctionKind::MoveAssignment:
+ return (llvm::Twine("") + ClassName + "(" + ClassName + " &&) = " +
+ deleteOrDefault(MatchedDecl) + ";\n")
+ .str();
+ }
+};
+
+void RuleOfFiveCheck::addDiagnosticAndFixIt(
+ const MatchFinder::MatchResult &Result, const CXXMethodDecl &MatchedDecl,
+ SpecialFunctionKind S) {
+ const CXXRecordDecl *Class = MatchedDecl.getParent();
+
+ DiagnosticBuilder Diag =
+ diag(MatchedDecl.getLocation(), getDiagnosticFormat(S)) << Class;
+
+ if (!getLangOpts().CPlusPlus11)
+ return;
+
+ SourceLocation FixitLocation = getInsertionLocation(MatchedDecl, Result, S);
+ if (FixitLocation.isInvalid())
+ return;
+
+ std::string FixIt = buildFixIt(MatchedDecl, Class->getName(), S);
+
+ Diag << FixItHint::CreateInsertion(FixitLocation, FixIt);
+}
+
+void RuleOfFiveCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ if (const auto *MatchedDecl =
+ Result.Nodes.getNodeAs<CXXConstructorDecl>("copy-ctor"))
+ addDiagnosticAndFixIt(Result, *MatchedDecl,
+ SpecialFunctionKind::CopyConstructor);
+ else if (const auto *MatchedDecl =
+ Result.Nodes.getNodeAs<CXXMethodDecl>("copy assignment"))
+ addDiagnosticAndFixIt(Result, *MatchedDecl,
+ SpecialFunctionKind::CopyAssignment);
+ else if (const auto *MatchedDecl =
+ Result.Nodes.getNodeAs<CXXConstructorDecl>("move-ctor"))
+ addDiagnosticAndFixIt(Result, *MatchedDecl,
+ SpecialFunctionKind::MoveConstructor);
+ else if (const auto *MatchedDecl =
+ Result.Nodes.getNodeAs<CXXMethodDecl>("move assignment"))
+ addDiagnosticAndFixIt(Result, *MatchedDecl,
+ SpecialFunctionKind::MoveAssignment);
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/misc/MiscTidyModule.cpp
===================================================================
--- clang-tidy/misc/MiscTidyModule.cpp
+++ clang-tidy/misc/MiscTidyModule.cpp
@@ -34,6 +34,7 @@
#include "UnusedAliasDeclsCheck.h"
#include "UnusedParametersCheck.h"
#include "UnusedRAIICheck.h"
+#include "RuleOfFiveCheck.h"
#include "VirtualNearMissCheck.h"
namespace clang {
@@ -88,6 +89,8 @@
CheckFactories.registerCheck<UnusedParametersCheck>(
"misc-unused-parameters");
CheckFactories.registerCheck<UnusedRAIICheck>("misc-unused-raii");
+ CheckFactories.registerCheck<RuleOfFiveCheck>(
+ "misc-rule-of-five");
CheckFactories.registerCheck<VirtualNearMissCheck>(
"misc-virtual-near-miss");
}
Index: clang-tidy/misc/CMakeLists.txt
===================================================================
--- clang-tidy/misc/CMakeLists.txt
+++ clang-tidy/misc/CMakeLists.txt
@@ -26,6 +26,7 @@
UnusedParametersCheck.cpp
UnusedRAIICheck.cpp
UniqueptrResetReleaseCheck.cpp
+ RuleOfFiveCheck.cpp
VirtualNearMissCheck.cpp
LINK_LIBS
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
