Index: clang-tidy/misc/AssignmentAndConstructionCheck.h =================================================================== --- /dev/null +++ clang-tidy/misc/AssignmentAndConstructionCheck.h @@ -0,0 +1,48 @@ +//===--- AssignmentAndConstructionCheck.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_ASSIGNMENT_AND_CONSTRUCTION_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_ASSIGNMENT_AND_CONSTRUCTION_H + +#include "../ClangTidy.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-assignment-and-construction.html +class AssignmentAndConstructionCheck : public ClangTidyCheck { +public: + AssignmentAndConstructionCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + + enum class SpecialFunctionKind { + CopyConstructor, + CopyAssignment, + MoveConstructor, + MoveAssignment + }; + +private: + void diagnosticAndFixIt( + 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_ASSIGNMENT_AND_CONSTRUCTION_H Index: clang-tidy/misc/AssignmentAndConstructionCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/misc/AssignmentAndConstructionCheck.cpp @@ -0,0 +1,174 @@ +//===--- AssignmentAndConstructionCheck.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 "AssignmentAndConstructionCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace misc { + +void AssignmentAndConstructionCheck::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); +} + +static StringRef deleteOrDefault(const CXXMethodDecl &MethodDecl) { + return MethodDecl.isDefaulted() ? "default" : "delete"; +} + +static SourceLocation +getInsertionLocation(const CXXMethodDecl &MatchedDecl, + const MatchFinder::MatchResult &Result, + AssignmentAndConstructionCheck::SpecialFunctionKind S) { + switch (S) { + case AssignmentAndConstructionCheck::SpecialFunctionKind::CopyConstructor: + case AssignmentAndConstructionCheck::SpecialFunctionKind::MoveConstructor: { + SourceLocation DeclEnd = Lexer::getLocForEndOfToken( + MatchedDecl.getLocEnd(), 0, *Result.SourceManager, + Result.Context->getLangOpts()); + return DeclEnd.getLocWithOffset(1); + } + case AssignmentAndConstructionCheck::SpecialFunctionKind::CopyAssignment: + case AssignmentAndConstructionCheck::SpecialFunctionKind::MoveAssignment: { + SourceLocation DeclBegin = MatchedDecl.getLocStart(); + return DeclBegin.getLocWithOffset(-1); + } + } +} + +StringRef diagnosticFormat(AssignmentAndConstructionCheck::SpecialFunctionKind S) { + switch (S) { + case AssignmentAndConstructionCheck::SpecialFunctionKind::CopyConstructor: + return "class '%0' defines a copy-constructor but not a copy-assignment " + "operator"; + case AssignmentAndConstructionCheck::SpecialFunctionKind::CopyAssignment: + return "class '%0' defines a copy-assignment operator but not a " + "copy-constructor"; + case AssignmentAndConstructionCheck::SpecialFunctionKind::MoveConstructor: + return "class '%0' defines a move-constructor but not a move-assignment " + "operator"; + case AssignmentAndConstructionCheck::SpecialFunctionKind::MoveAssignment: + return "class '%0' defines a move-assignment operator but not a " + "move-constructor"; + } +}; + +static std::string buildFixIt(const CXXMethodDecl &MatchedDecl, StringRef ClassName, + AssignmentAndConstructionCheck::SpecialFunctionKind S) { + switch (S) { + case AssignmentAndConstructionCheck::SpecialFunctionKind::CopyConstructor: + return (llvm::Twine("\n") + ClassName + " &operator=(const " + ClassName + + " &) = " + deleteOrDefault(MatchedDecl) + ";") + .str(); + case AssignmentAndConstructionCheck::SpecialFunctionKind::CopyAssignment: + return (llvm::Twine("") + ClassName + "(const " + ClassName + " &) = " + + deleteOrDefault(MatchedDecl) + ";\n") + .str(); + case AssignmentAndConstructionCheck::SpecialFunctionKind::MoveConstructor: + return (llvm::Twine("\n") + ClassName + " &operator=(" + ClassName + + " &&) = " + deleteOrDefault(MatchedDecl) + ";") + .str(); + case AssignmentAndConstructionCheck::SpecialFunctionKind::MoveAssignment: + return (llvm::Twine("") + ClassName + "(" + ClassName + " &&) = " + + deleteOrDefault(MatchedDecl) + ";\n") + .str(); + } +}; + +void AssignmentAndConstructionCheck::diagnosticAndFixIt( + const MatchFinder::MatchResult &Result, const CXXMethodDecl &MatchedDecl, + SpecialFunctionKind S) { + StringRef ClassName = MatchedDecl.getParent()->getName(); + + DiagnosticBuilder Diag = diag(MatchedDecl.getLocation(), diagnosticFormat(S)) + << ClassName; + + if (!getLangOpts().CPlusPlus11) + return; + + SourceLocation FixitLocation = getInsertionLocation(MatchedDecl, Result, S); + if (FixitLocation.isInvalid()) + return; + + std::string FixIt = buildFixIt(MatchedDecl, ClassName, S); (llvm::Twine("\n") + ClassName + " &operator=(const " + + ClassName + " &) = " + deleteOrDefault(MatchedDecl) + ";") + .str(); + + Diag << FixItHint::CreateInsertion(FixitLocation, FixIt); +} + +void AssignmentAndConstructionCheck::check( + const MatchFinder::MatchResult &Result) { + if (const auto *MatchedDecl = + Result.Nodes.getNodeAs("copy-ctor")) { + diagnosticAndFixIt(Result, *MatchedDecl, + SpecialFunctionKind::CopyConstructor); + } else if (const auto *MatchedDecl = + Result.Nodes.getNodeAs("copy-assignment")) { + diagnosticAndFixIt(Result, *MatchedDecl, + SpecialFunctionKind::CopyAssignment); + } + + else if (const auto *MatchedDecl = + Result.Nodes.getNodeAs("move-ctor")) { + diagnosticAndFixIt(Result, *MatchedDecl, + SpecialFunctionKind::MoveConstructor); + } else if (const auto *MatchedDecl = + Result.Nodes.getNodeAs("move-assignment")) { + diagnosticAndFixIt(Result, *MatchedDecl, + SpecialFunctionKind::MoveAssignment); + } +} + +} // namespace misc +} // namespace tidy +} // namespace clang 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 + AssignmentAndConstructionCheck.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 "AssignmentAndConstructionCheck.h" #include "VirtualNearMissCheck.h" namespace clang { @@ -88,6 +89,8 @@ CheckFactories.registerCheck( "misc-unused-parameters"); CheckFactories.registerCheck("misc-unused-raii"); + CheckFactories.registerCheck( + "misc-assignment-and-construction"); CheckFactories.registerCheck( "misc-virtual-near-miss"); } Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -65,6 +65,7 @@ misc-unused-alias-decls misc-unused-parameters misc-unused-raii + misc-assignment-and-construction misc-virtual-near-miss modernize-loop-convert modernize-make-unique Index: docs/clang-tidy/checks/misc-assignment-and-construction.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/misc-assignment-and-construction.rst @@ -0,0 +1,25 @@ +.. title:: clang-tidy - misc-assignment-and-construction + +misc-assignment-and-construction +========================================= + +Compilers will generate an assignment operator even if the user defines a copy +constructor. This behaviour is deprecated by the standard (C++ 14 draft +standard [class.construction 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. Incorrect +compiler-generated assignment/construction can cause unexpected behaviour. An +explicitly deleted constructor or assignment operator will cause a compiler +error if it is used. + +Where the user has defaulted one of the pair, the other will be explictly +defaulted. + Index: test/clang-tidy/misc-assignment-and-construction.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-assignment-and-construction.cpp @@ -0,0 +1,161 @@ +// RUN: %check_clang_tidy %s misc-assignment-and-construction %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-assignment-and-construction] +}; + +// 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-assignment-and-construction] +}; + +// 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; +}; + +// +// 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-assignment-and-construction] +}; + +// 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-assignment-and-construction] +}; + +// 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 &); +}; + +// +// 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-assignment-and-construction] +}; + +// 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-assignment-and-construction] +}; + +// 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; +}; + +// +// 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-assignment-and-construction] +}; + +// 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-assignment-and-construction] +}; + +// 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 &&); +};