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 + UserDefinedCopyWithoutAssignmentCheck.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 "UserDefinedCopyWithoutAssignmentCheck.h" #include "VirtualNearMissCheck.h" namespace clang { @@ -88,6 +89,8 @@ CheckFactories.registerCheck( "misc-unused-parameters"); CheckFactories.registerCheck("misc-unused-raii"); + CheckFactories.registerCheck( + "misc-user-defined-copy-without-assignment"); CheckFactories.registerCheck( "misc-virtual-near-miss"); } Index: clang-tidy/misc/UserDefinedCopyWithoutAssignmentCheck.h =================================================================== --- /dev/null +++ clang-tidy/misc/UserDefinedCopyWithoutAssignmentCheck.h @@ -0,0 +1,38 @@ +//===--- UserDefinedCopyWithoutAssignmentCheck.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_USER_DEFINED_COPY_WITHOUT_ASSIGNMENT_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_USER_DEFINED_COPY_WITHOUT_ASSIGNMENT_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace misc { + +/// Finds user-defined copy-constructors but no assignement operator. +/// +/// MSVC 2015 will generate an assignment operator even if the user defines a +/// copy-constructor. This check finds classes with user-defined +/// copy-constructors but no assignement operator and (defensively) defines the +/// assignment operator to be `= delete`. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-user-defined-copy-without-assignment.html +class UserDefinedCopyWithoutAssignmentCheck : public ClangTidyCheck { public: + UserDefinedCopyWithoutAssignmentCheck(StringRef Name, ClangTidyContext + *Context) : ClangTidyCheck(Name, Context) {} void + registerMatchers(ast_matchers::MatchFinder *Finder) override; void + check(const ast_matchers::MatchFinder::MatchResult &Result) override; }; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_USER_DEFINED_COPY_WITHOUT_ASSIGNMENT_H Index: clang-tidy/misc/UserDefinedCopyWithoutAssignmentCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/misc/UserDefinedCopyWithoutAssignmentCheck.cpp @@ -0,0 +1,176 @@ +//===--- UserDefinedCopyWithoutAssignmentCheck.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 "UserDefinedCopyWithoutAssignmentCheck.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 UserDefinedCopyWithoutAssignmentCheck::registerMatchers( + MatchFinder *Finder) { + 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); + + 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"; +} + +void UserDefinedCopyWithoutAssignmentCheck::check( + const MatchFinder::MatchResult &Result) { + if (const auto *MatchedDecl = + Result.Nodes.getNodeAs("copy-ctor")) { + // + // Copy-constructor + // + StringRef ClassName = MatchedDecl->getParent()->getName(); + + DiagnosticBuilder Diag = diag(MatchedDecl->getLocation(), + "class '%0' defines a copy-constructor " + "but not a copy-assignment operator") + << ClassName; + + if (!getLangOpts().CPlusPlus11) + return; + + SourceLocation CCtorEnd = Lexer::getLocForEndOfToken( + MatchedDecl->getLocEnd(), 0, *Result.SourceManager, + Result.Context->getLangOpts()); + CCtorEnd = CCtorEnd.getLocWithOffset(1); + if (CCtorEnd.isInvalid()) + return; + + auto Insertion = (llvm::Twine("\n") + ClassName + " &operator=(const " + + ClassName + " &) = " + deleteOrDefault(MatchedDecl) + ";") + .str(); + + Diag << FixItHint::CreateInsertion(CCtorEnd, Insertion); + } else if (const auto *MatchedDecl = + Result.Nodes.getNodeAs("copy-assignment")) { + // + // Copy-assignment + // + StringRef ClassName = MatchedDecl->getParent()->getName(); + + DiagnosticBuilder Diag = + diag(MatchedDecl->getLocation(), "class '%0' defines a copy-assignment " + "operator but not a copy-constructor") + << ClassName; + + if (!getLangOpts().CPlusPlus11) + return; + + SourceLocation AssignmentBegin = MatchedDecl->getLocStart(); + auto BeforeAssignment = AssignmentBegin.getLocWithOffset(-1); + if (BeforeAssignment.isInvalid()) + return; + + auto Insertion = (llvm::Twine("") + ClassName + "(const " + ClassName + + " &) = " + deleteOrDefault(MatchedDecl) + ";\n") + .str(); + + Diag << FixItHint::CreateInsertion(BeforeAssignment, Insertion); + } + + else if (const auto *MatchedDecl = + Result.Nodes.getNodeAs("move-ctor")) { + // + // Move-ctor + // + StringRef ClassName = MatchedDecl->getParent()->getName(); + + DiagnosticBuilder Diag = diag(MatchedDecl->getLocation(), + "class '%0' defines a move-constructor " + "but not a move-assignment operator") + << ClassName; + + if (!getLangOpts().CPlusPlus11) + return; + + SourceLocation CCtorEnd = Lexer::getLocForEndOfToken( + MatchedDecl->getLocEnd(), 0, *Result.SourceManager, + Result.Context->getLangOpts()); + CCtorEnd = CCtorEnd.getLocWithOffset(1); + if (CCtorEnd.isInvalid()) + return; + + auto Insertion = + (llvm::Twine("\n") + ClassName + " &operator=(" + ClassName + + " &&) = " + deleteOrDefault(MatchedDecl) + ";") + .str(); + + Diag << FixItHint::CreateInsertion(CCtorEnd, Insertion); + } else if (const auto *MatchedDecl = + Result.Nodes.getNodeAs("move-assignment")) { + // + // Move-assignment + // + StringRef ClassName = MatchedDecl->getParent()->getName(); + + DiagnosticBuilder Diag = + diag(MatchedDecl->getLocation(), "class '%0' defines a move-assignment " + "operator but not a move-constructor") + << ClassName; + + if (!getLangOpts().CPlusPlus11) + return; + + SourceLocation AssignmentBegin = MatchedDecl->getLocStart(); + auto BeforeAssignment = AssignmentBegin.getLocWithOffset(-1); + if (BeforeAssignment.isInvalid()) + return; + + auto Insertion = (llvm::Twine("") + ClassName + "(" + ClassName + + " &&) = " + deleteOrDefault(MatchedDecl) + ";\n") + .str(); + + Diag << FixItHint::CreateInsertion(BeforeAssignment, Insertion); + } +} + +} // 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 @@ -65,6 +65,7 @@ misc-unused-alias-decls misc-unused-parameters misc-unused-raii + misc-user-defined-copy-without-assignment misc-virtual-near-miss modernize-loop-convert modernize-make-unique Index: docs/clang-tidy/checks/misc-user-defined-copy-without-assignment.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/misc-user-defined-copy-without-assignment.rst @@ -0,0 +1,36 @@ +.. title:: clang-tidy - misc-user-defined-copy-without-assignment + +misc-user-defined-copy-without-assignment +========================================= + +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 12.8.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 with a user-defined (including deleted) +copy-constructor but no assignment operator. + + .. code:: c++ + class A { + A(const A&); + }; + +Will be matched and fixed to delete the assignment operator: + + .. code:: c++ + class A { + A(const A&); + A& operator = (const A&) = delete; + }; + +The fix is defensive. Incorrect compiler-generated assignement can cause +unexpected behaviour. An explicitly deleted assignment operator will cause a +compiler error if it is used. + Index: test/clang-tidy/misc-user-defined-copy-without-assignment.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-user-defined-copy-without-assignment.cpp @@ -0,0 +1,161 @@ +// RUN: %check_clang_tidy %s misc-user-defined-copy-without-assignment %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-user-defined-copy-without-assignment] +}; + +// 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-user-defined-copy-without-assignment] +}; + +// 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-user-defined-copy-without-assignment] +}; + +// 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-user-defined-copy-without-assignment] +}; + +// 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-user-defined-copy-without-assignment] +}; + +// 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-user-defined-copy-without-assignment] +}; + +// 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-user-defined-copy-without-assignment] +}; + +// 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-user-defined-copy-without-assignment] +}; + +// 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 &&); +};