Index: clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp +++ clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -10,6 +10,7 @@ #include "../ClangTidy.h" #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" +#include "CopyConstructorInitCheck.h" #include "IntegerDivisionCheck.h" #include "SuspiciousMemsetUsageCheck.h" #include "UndefinedMemoryManipulationCheck.h" @@ -21,6 +22,8 @@ class BugproneModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck( + "misc-copy-constructor-init"); CheckFactories.registerCheck( "bugprone-integer-division"); CheckFactories.registerCheck( Index: clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt =================================================================== --- clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt +++ clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt @@ -2,6 +2,7 @@ add_clang_library(clangTidyBugproneModule BugproneTidyModule.cpp + CopyConstructorInitCheck.cpp IntegerDivisionCheck.cpp SuspiciousMemsetUsageCheck.cpp UndefinedMemoryManipulationCheck.cpp Index: clang-tools-extra/trunk/clang-tidy/bugprone/CopyConstructorInitCheck.h =================================================================== --- clang-tools-extra/trunk/clang-tidy/bugprone/CopyConstructorInitCheck.h +++ clang-tools-extra/trunk/clang-tidy/bugprone/CopyConstructorInitCheck.h @@ -0,0 +1,36 @@ +//===--- CopyConstructorInitCheck.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_BUGPRONE_COPY_CONSTRUCTOR_INIT_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_COPY_CONSTRUCTOR_INIT_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +/// Finds copy constructors where the ctor don't call the copy constructor of +/// the base class. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-copy-constructor-init.html +class CopyConstructorInitCheck : public ClangTidyCheck { +public: + CopyConstructorInitCheck(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_BUGPRONE_COPY_CONSTRUCTOR_INIT_H Index: clang-tools-extra/trunk/clang-tidy/bugprone/CopyConstructorInitCheck.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/bugprone/CopyConstructorInitCheck.cpp +++ clang-tools-extra/trunk/clang-tidy/bugprone/CopyConstructorInitCheck.cpp @@ -0,0 +1,121 @@ +//===--- CopyConstructorInitCheck.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 "CopyConstructorInitCheck.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 bugprone { + +void CopyConstructorInitCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus) + return; + + // In the future this might be extended to move constructors? + Finder->addMatcher( + cxxConstructorDecl( + isCopyConstructor(), + hasAnyConstructorInitializer(cxxCtorInitializer( + isBaseInitializer(), + withInitializer(cxxConstructExpr(hasDeclaration( + cxxConstructorDecl(isDefaultConstructor())))))), + unless(isInstantiated())) + .bind("ctor"), + this); +} + +void CopyConstructorInitCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Ctor = Result.Nodes.getNodeAs("ctor"); + std::string ParamName = Ctor->getParamDecl(0)->getNameAsString(); + + // We want only one warning (and FixIt) for each ctor. + std::string FixItInitList; + bool HasRelevantBaseInit = false; + bool ShouldNotDoFixit = false; + bool HasWrittenInitializer = false; + SmallVector SafeFixIts; + for (const auto *Init : Ctor->inits()) { + bool CtorInitIsWritten = Init->isWritten(); + HasWrittenInitializer = HasWrittenInitializer || CtorInitIsWritten; + if (!Init->isBaseInitializer()) + continue; + const Type *BaseType = Init->getBaseClass(); + // Do not do fixits if there is a type alias involved or one of the bases + // are explicitly initialized. In the latter case we not do fixits to avoid + // -Wreorder warnings. + if (const auto *TempSpecTy = dyn_cast(BaseType)) + ShouldNotDoFixit = ShouldNotDoFixit || TempSpecTy->isTypeAlias(); + ShouldNotDoFixit = ShouldNotDoFixit || isa(BaseType); + ShouldNotDoFixit = ShouldNotDoFixit || CtorInitIsWritten; + const CXXRecordDecl *BaseClass = + BaseType->getAsCXXRecordDecl()->getDefinition(); + if (BaseClass->field_empty() && + BaseClass->forallBases( + [](const CXXRecordDecl *Class) { return Class->field_empty(); })) + continue; + bool NonCopyableBase = false; + for (const auto *Ctor : BaseClass->ctors()) { + if (Ctor->isCopyConstructor() && + (Ctor->getAccess() == AS_private || Ctor->isDeleted())) { + NonCopyableBase = true; + break; + } + } + if (NonCopyableBase) + continue; + const auto *CExpr = dyn_cast(Init->getInit()); + if (!CExpr || !CExpr->getConstructor()->isDefaultConstructor()) + continue; + HasRelevantBaseInit = true; + if (CtorInitIsWritten) { + if (!ParamName.empty()) + SafeFixIts.push_back( + FixItHint::CreateInsertion(CExpr->getLocEnd(), ParamName)); + } else { + if (Init->getSourceLocation().isMacroID() || + Ctor->getLocation().isMacroID() || ShouldNotDoFixit) + break; + FixItInitList += BaseClass->getNameAsString(); + FixItInitList += "(" + ParamName + "), "; + } + } + if (!HasRelevantBaseInit) + return; + + auto Diag = diag(Ctor->getLocation(), + "calling a base constructor other than the copy constructor") + << SafeFixIts; + + if (FixItInitList.empty() || ParamName.empty() || ShouldNotDoFixit) + return; + + std::string FixItMsg{FixItInitList.substr(0, FixItInitList.size() - 2)}; + SourceLocation FixItLoc; + // There is no initialization list in this constructor. + if (!HasWrittenInitializer) { + FixItLoc = Ctor->getBody()->getLocStart(); + FixItMsg = " : " + FixItMsg; + } else { + // We apply the missing ctors at the beginning of the initialization list. + FixItLoc = (*Ctor->init_begin())->getSourceLocation(); + FixItMsg += ','; + } + FixItMsg += ' '; + + Diag << FixItHint::CreateInsertion(FixItLoc, FixItMsg); +} // namespace misc + +} // namespace misc +} // namespace tidy +} // namespace clang Index: clang-tools-extra/trunk/docs/ReleaseNotes.rst =================================================================== --- clang-tools-extra/trunk/docs/ReleaseNotes.rst +++ clang-tools-extra/trunk/docs/ReleaseNotes.rst @@ -136,6 +136,11 @@ Checks if the required file flag ``MFD_CLOEXEC`` is present in the argument of ``memfd_create()``. +- New `bugprone-copy-constructor-init + `_ check + + Finds copy constructors which don't call the copy constructor of the base class. + - New `bugprone-integer-division `_ check Index: clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-copy-constructor-init.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-copy-constructor-init.rst +++ clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-copy-constructor-init.rst @@ -0,0 +1,29 @@ +.. title:: clang-tidy - bugprone-copy-constructor-init + +bugprone-copy-constructor-init +============================== + +Finds copy constructors where the constructor doesn't call +the copy constructor of the base class. + +.. code-block:: c++ + + class Copyable { + public: + Copyable() = default; + Copyable(const Copyable &) = default; + }; + class X2 : public Copyable { + X2(const X2 &other) {} // Copyable(other) is missing + }; + +Also finds copy constructors where the constructor of +the base class don't have parameter. + +.. code-block:: c++ + + class X4 : public Copyable { + X4(const X4 &other) : Copyable() {} // other is missing + }; + +The check also suggests a fix-its in some cases. Index: clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst +++ clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst @@ -17,6 +17,7 @@ android-cloexec-open android-cloexec-socket boost-use-to-string + bugprone-copy-constructor-init bugprone-integer-division bugprone-suspicious-memset-usage bugprone-undefined-memory-manipulation Index: clang-tools-extra/trunk/test/clang-tidy/bugprone-copy-constructor-init.cpp =================================================================== --- clang-tools-extra/trunk/test/clang-tidy/bugprone-copy-constructor-init.cpp +++ clang-tools-extra/trunk/test/clang-tidy/bugprone-copy-constructor-init.cpp @@ -0,0 +1,188 @@ +// RUN: %check_clang_tidy %s misc-copy-constructor-init %t + +class NonCopyable { +public: + NonCopyable() = default; + NonCopyable(const NonCopyable &) = delete; + +private: + int a; +}; + +class NonCopyable2 { +public: + NonCopyable2() = default; + +private: + NonCopyable2(const NonCopyable2 &); + int a; +}; + +class Copyable { +public: + Copyable() = default; + Copyable(const Copyable &) = default; + +private: + int a; +}; + +class Copyable2 { +public: + Copyable2() = default; + Copyable2(const Copyable2 &) = default; + +private: + int a; +}; + +class Copyable3 : public Copyable { +public: + Copyable3() = default; + Copyable3(const Copyable3 &) = default; +}; + +template +class Copyable4 { +public: + Copyable4() = default; + Copyable4(const Copyable4 &) = default; + +private: + int a; +}; + +template +class Copyable5 { +public: + Copyable5() = default; + Copyable5(const Copyable5 &) = default; + +private: + int a; +}; + +class EmptyCopyable { +public: + EmptyCopyable() = default; + EmptyCopyable(const EmptyCopyable &) = default; +}; + +template +using CopyableAlias = Copyable5; + +typedef Copyable5 CopyableAlias2; + +class X : public Copyable, public EmptyCopyable { + X(const X &other) : Copyable(other) {} +}; + +class X2 : public Copyable2 { + X2(const X2 &other) {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor other than the copy constructor [misc-copy-constructor-init] + // CHECK-FIXES: X2(const X2 &other) : Copyable2(other) {} +}; + +class X2_A : public Copyable2 { + X2_A(const X2_A &) {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor + // CHECK-FIXES: X2_A(const X2_A &) {} +}; + +class X3 : public Copyable, public Copyable2 { + X3(const X3 &other) : Copyable(other) {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor + // CHECK-FIXES: X3(const X3 &other) : Copyable(other) {} +}; + +class X4 : public Copyable { + X4(const X4 &other) : Copyable() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor + // CHECK-FIXES: X4(const X4 &other) : Copyable(other) {} +}; + +class X5 : public Copyable3 { + X5(const X5 &other) {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor + // CHECK-FIXES: X5(const X5 &other) : Copyable3(other) {} +}; + +class X6 : public Copyable2, public Copyable3 { + X6(const X6 &other) {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor + // CHECK-FIXES: X6(const X6 &other) : Copyable2(other), Copyable3(other) {} +}; + +class X7 : public Copyable, public Copyable2 { + X7(const X7 &other) : Copyable() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor + // CHECK-FIXES: X7(const X7 &other) : Copyable(other) {} +}; + +class X8 : public Copyable4 { + X8(const X8 &other) : Copyable4(other) {} +}; + +class X9 : public Copyable4 { + X9(const X9 &other) : Copyable4() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor + // CHECK-FIXES: X9(const X9 &other) : Copyable4(other) {} +}; + +class X10 : public Copyable4 { + X10(const X10 &other) {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor + // CHECK-FIXES: X10(const X10 &other) : Copyable4(other) {} +}; + +class X11 : public Copyable5 { + X11(const X11 &other) {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor + // CHECK-FIXES: X11(const X11 &other) : Copyable5(other) {} +}; + +class X12 : public CopyableAlias { + X12(const X12 &other) {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor + // CHECK-FIXES: X12(const X12 &other) {} +}; + +template +class X13 : T { + X13(const X13 &other) {} +}; + +template class X13; +template class X13; + +#define FROMMACRO \ + class X14 : public Copyable2 { \ + X14(const X14 &other) {} \ + }; + +FROMMACRO +// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: calling a base constructor + +class X15 : public CopyableAlias2 { + X15(const X15 &other) {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor + // CHECK-FIXES: X15(const X15 &other) {} +}; + +class X16 : public NonCopyable { + X16(const X16 &other) {} +}; + +class X17 : public NonCopyable2 { + X17(const X17 &other) {} +}; + +class X18 : private Copyable { + X18(const X18 &other) {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor + // CHECK-FIXES: X18(const X18 &other) : Copyable(other) {} +}; + +class X19 : private Copyable { + X19(const X19 &other) : Copyable(other) {} +};