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 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( "misc-unused-parameters"); CheckFactories.registerCheck("misc-unused-raii"); + CheckFactories.registerCheck( + "misc-rule-of-five"); CheckFactories.registerCheck( "misc-virtual-near-miss"); } 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("copy-ctor")) + addDiagnosticAndFixIt(Result, *MatchedDecl, + SpecialFunctionKind::CopyConstructor); + else if (const auto *MatchedDecl = + Result.Nodes.getNodeAs("copy assignment")) + addDiagnosticAndFixIt(Result, *MatchedDecl, + SpecialFunctionKind::CopyAssignment); + else if (const auto *MatchedDecl = + Result.Nodes.getNodeAs("move-ctor")) + addDiagnosticAndFixIt(Result, *MatchedDecl, + SpecialFunctionKind::MoveConstructor); + else if (const auto *MatchedDecl = + Result.Nodes.getNodeAs("move assignment")) + addDiagnosticAndFixIt(Result, *MatchedDecl, + SpecialFunctionKind::MoveAssignment); +} + +} // namespace misc +} // namespace tidy +} // namespace clang 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: 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: 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: }; +