diff options
8 files changed, 475 insertions, 0 deletions
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt index d466ca83566..ea97ebe2dee 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt @@ -7,6 +7,7 @@ add_clang_library(clangTidyCppCoreGuidelinesModule ProBoundsPointerArithmeticCheck.cpp ProTypeConstCastCheck.cpp ProTypeCstyleCastCheck.cpp + ProTypeMemberInitCheck.cpp ProTypeReinterpretCastCheck.cpp ProTypeStaticCastDowncastCheck.cpp ProTypeUnionAccessCheck.cpp diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp index db783a3975d..70fa0f493a5 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp @@ -16,6 +16,7 @@ #include "ProBoundsPointerArithmeticCheck.h" #include "ProTypeConstCastCheck.h" #include "ProTypeCstyleCastCheck.h" +#include "ProTypeMemberInitCheck.h" #include "ProTypeReinterpretCastCheck.h" #include "ProTypeStaticCastDowncastCheck.h" #include "ProTypeUnionAccessCheck.h" @@ -39,6 +40,8 @@ public: "cppcoreguidelines-pro-type-const-cast"); CheckFactories.registerCheck<ProTypeCstyleCastCheck>( "cppcoreguidelines-pro-type-cstyle-cast"); + CheckFactories.registerCheck<ProTypeMemberInitCheck>( + "cppcoreguidelines-pro-type-member-init"); CheckFactories.registerCheck<ProTypeReinterpretCastCheck>( "cppcoreguidelines-pro-type-reinterpret-cast"); CheckFactories.registerCheck<ProTypeStaticCastDowncastCheck>( diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp new file mode 100644 index 00000000000..6e5896a9f49 --- /dev/null +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp @@ -0,0 +1,231 @@ +//===--- ProTypeMemberInitCheck.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 "ProTypeMemberInitCheck.h" +#include "../utils/LexerUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" +#include "llvm/ADT/SmallPtrSet.h" + +using namespace clang::ast_matchers; +using llvm::SmallPtrSet; +using llvm::SmallPtrSetImpl; + +namespace clang { +namespace tidy { +namespace cppcoreguidelines { + +namespace { + +AST_MATCHER(CXXConstructorDecl, isUserProvided) { + return Node.isUserProvided(); +} + +static void +fieldsRequiringInit(const RecordDecl::field_range &Fields, + SmallPtrSetImpl<const FieldDecl *> &FieldsToInit) { + for (const FieldDecl *F : Fields) { + QualType Type = F->getType(); + if (Type->isPointerType() || Type->isBuiltinType()) + FieldsToInit.insert(F); + } +} + +void removeFieldsInitializedInBody( + const Stmt &Stmt, ASTContext &Context, + SmallPtrSetImpl<const FieldDecl *> &FieldDecls) { + auto Matches = + match(findAll(binaryOperator( + hasOperatorName("="), + hasLHS(memberExpr(member(fieldDecl().bind("fieldDecl")))))), + Stmt, Context); + for (const auto &Match : Matches) + FieldDecls.erase(Match.getNodeAs<FieldDecl>("fieldDecl")); +} + +// Creates comma separated list of fields requiring initialization in order of +// declaration. +std::string toCommaSeparatedString( + const RecordDecl::field_range &FieldRange, + const SmallPtrSetImpl<const FieldDecl *> &FieldsRequiringInit) { + std::string List; + llvm::raw_string_ostream Stream(List); + size_t AddedFields = 0; + for (const FieldDecl *Field : FieldRange) { + if (FieldsRequiringInit.count(Field) > 0) { + Stream << Field->getName(); + if (++AddedFields < FieldsRequiringInit.size()) + Stream << ", "; + } + } + return Stream.str(); +} + +// Contains all fields in correct order that need to be inserted at the same +// location for pre C++11. +// There are 3 kinds of insertions: +// 1. The fields are inserted after an existing CXXCtorInitializer stored in +// InitializerBefore. This will be the case whenever there is a written +// initializer before the fields available. +// 2. The fields are inserted before the first existing initializer stored in +// InitializerAfter. +// 3. There are no written initializers and the fields will be inserted before +// the constructor's body creating a new initializer list including the ':'. +struct FieldsInsertion { + const CXXCtorInitializer *InitializerBefore; + const CXXCtorInitializer *InitializerAfter; + SmallVector<const FieldDecl *, 4> Fields; + + SourceLocation getLocation(const ASTContext &Context, + const CXXConstructorDecl &Constructor) const { + if (InitializerBefore != nullptr) { + return Lexer::getLocForEndOfToken(InitializerBefore->getRParenLoc(), 0, + Context.getSourceManager(), + Context.getLangOpts()); + } + auto StartLocation = InitializerAfter != nullptr + ? InitializerAfter->getSourceRange().getBegin() + : Constructor.getBody()->getLocStart(); + auto Token = + lexer_utils::getPreviousNonCommentToken(Context, StartLocation); + return Lexer::getLocForEndOfToken(Token.getLocation(), 0, + Context.getSourceManager(), + Context.getLangOpts()); + } + + std::string codeToInsert() const { + assert(!Fields.empty() && "No fields to insert"); + std::string Code; + llvm::raw_string_ostream Stream(Code); + // Code will be inserted before the first written initializer after ':', + // append commas. + if (InitializerAfter != nullptr) { + for (const auto *Field : Fields) + Stream << " " << Field->getName() << "(),"; + } else { + // The full initializer list is created, add extra space after + // constructor's rparens. + if (InitializerBefore == nullptr) + Stream << " "; + for (const auto *Field : Fields) + Stream << ", " << Field->getName() << "()"; + } + Stream.flush(); + // The initializer list is created, replace leading comma with colon. + if (InitializerBefore == nullptr && InitializerAfter == nullptr) + Code[1] = ':'; + return Code; + } +}; + +SmallVector<FieldsInsertion, 16> computeInsertions( + const CXXConstructorDecl::init_const_range &Inits, + const RecordDecl::field_range &Fields, + const SmallPtrSetImpl<const FieldDecl *> &FieldsRequiringInit) { + // Find last written non-member initializer or null. + const CXXCtorInitializer *LastWrittenNonMemberInit = nullptr; + for (const CXXCtorInitializer *Init : Inits) { + if (Init->isWritten() && !Init->isMemberInitializer()) + LastWrittenNonMemberInit = Init; + } + SmallVector<FieldsInsertion, 16> OrderedFields; + OrderedFields.push_back({LastWrittenNonMemberInit, nullptr, {}}); + + auto CurrentField = Fields.begin(); + for (const CXXCtorInitializer *Init : Inits) { + if (Init->isWritten() && Init->isMemberInitializer()) { + const FieldDecl *MemberField = Init->getMember(); + // Add all fields between current field and this member field the previous + // FieldsInsertion if the field requires initialization. + for (; CurrentField != Fields.end() && *CurrentField != MemberField; + ++CurrentField) { + if (FieldsRequiringInit.count(*CurrentField) > 0) + OrderedFields.back().Fields.push_back(*CurrentField); + } + // If this is the first written member initializer and there was no + // written non-member initializer set this initializer as + // InitializerAfter. + if (OrderedFields.size() == 1 && + OrderedFields.back().InitializerBefore == nullptr) + OrderedFields.back().InitializerAfter = Init; + OrderedFields.push_back({Init, nullptr, {}}); + } + } + // Add remaining fields that require initialization to last FieldsInsertion. + for (; CurrentField != Fields.end(); ++CurrentField) { + if (FieldsRequiringInit.count(*CurrentField) > 0) + OrderedFields.back().Fields.push_back(*CurrentField); + } + return OrderedFields; +} + +} // namespace + +void ProTypeMemberInitCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher(cxxConstructorDecl(isDefinition(), isUserProvided(), + unless(isInstantiated())) + .bind("ctor"), + this); +} + +void ProTypeMemberInitCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Ctor = Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor"); + const auto &MemberFields = Ctor->getParent()->fields(); + + SmallPtrSet<const FieldDecl *, 16> FieldsToInit; + fieldsRequiringInit(MemberFields, FieldsToInit); + if (FieldsToInit.empty()) + return; + + for (CXXCtorInitializer *Init : Ctor->inits()) { + // Return early if this constructor simply delegates to another constructor + // in the same class. + if (Init->isDelegatingInitializer()) + return; + if (!Init->isMemberInitializer()) + continue; + FieldsToInit.erase(Init->getMember()); + } + removeFieldsInitializedInBody(*Ctor->getBody(), *Result.Context, + FieldsToInit); + if (FieldsToInit.empty()) + return; + + DiagnosticBuilder Diag = + diag(Ctor->getLocStart(), + "constructor does not initialize these built-in/pointer fields: %0") + << toCommaSeparatedString(MemberFields, FieldsToInit); + // Do not propose fixes in macros since we cannot place them correctly. + if (Ctor->getLocStart().isMacroID()) + return; + // For C+11 use in-class initialization which covers all future constructors + // as well. + if (Result.Context->getLangOpts().CPlusPlus11) { + for (const auto *Field : FieldsToInit) { + Diag << FixItHint::CreateInsertion( + Lexer::getLocForEndOfToken(Field->getSourceRange().getEnd(), 0, + Result.Context->getSourceManager(), + Result.Context->getLangOpts()), + "{}"); + } + return; + } + for (const auto &FieldsInsertion : + computeInsertions(Ctor->inits(), MemberFields, FieldsToInit)) { + if (!FieldsInsertion.Fields.empty()) + Diag << FixItHint::CreateInsertion( + FieldsInsertion.getLocation(*Result.Context, *Ctor), + FieldsInsertion.codeToInsert()); + } +} + +} // namespace cppcoreguidelines +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h new file mode 100644 index 00000000000..60765b82750 --- /dev/null +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h @@ -0,0 +1,43 @@ +//===--- ProTypeMemberInitCheck.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_CPPCOREGUIDELINES_PRO_TYPE_MEMBER_INIT_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_TYPE_MEMBER_INIT_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace cppcoreguidelines { + +/// \brief Checks that builtin or pointer fields are initialized by +/// user-defined constructors. +/// +/// Members initialized through function calls in the body of the constructor +/// will result in false positives. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-pro-type-member-init.html +/// TODO: See if 'fixes' for false positives are optimized away by the compiler. +/// TODO: "Issue a diagnostic when constructing an object of a trivially +/// constructible type without () or {} to initialize its members. To fix: Add +/// () or {}." +class ProTypeMemberInitCheck : public ClangTidyCheck { +public: + ProTypeMemberInitCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace cppcoreguidelines +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_TYPE_MEMBER_INIT_H diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-pro-type-member-init.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-pro-type-member-init.rst new file mode 100644 index 00000000000..6df400623be --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-pro-type-member-init.rst @@ -0,0 +1,18 @@ +.. title:: clang-tidy - cppcoreguidelines-pro-type-member-init + +cppcoreguidelines-pro-type-member-init +====================================== + +The check flags user-defined constructor definitions that do not initialize all +builtin and pointer fields which leaves their memory in an undefined state. + +For C++11 it suggests fixes to add in-class field initializers. For older +versions it inserts the field initializers into the constructor initializer +list. + +The check takes assignment of fields in the constructor body into account but +generates false positives for fields initialized in methods invoked in the +constructor body. + +This rule is part of the "Type safety" profile of the C++ Core Guidelines, see +https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Pro-type-memberinit. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 629dd603a69..4c2795e29ba 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -19,6 +19,7 @@ Clang-Tidy Checks cppcoreguidelines-pro-bounds-pointer-arithmetic cppcoreguidelines-pro-type-const-cast cppcoreguidelines-pro-type-cstyle-cast + cppcoreguidelines-pro-type-member-init cppcoreguidelines-pro-type-reinterpret-cast cppcoreguidelines-pro-type-static-cast-downcast cppcoreguidelines-pro-type-union-access diff --git a/clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init-cxx98.cpp b/clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init-cxx98.cpp new file mode 100644 index 00000000000..164ab56637e --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init-cxx98.cpp @@ -0,0 +1,67 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-member-init %t -- -- -std=c++98 + +struct PositiveFieldBeforeConstructor { + int F; + PositiveFieldBeforeConstructor() /* some comment */ {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these built-in/pointer fields: F + // CHECK-FIXES: PositiveFieldBeforeConstructor() : F() /* some comment */ {} +}; + +struct PositiveFieldAfterConstructor { + PositiveFieldAfterConstructor() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these built-in/pointer fields: F, G, H + // CHECK-FIXES: PositiveFieldAfterConstructor() : F(), G(), H() {} + int F; + bool G /* with comment */; + int *H; + PositiveFieldBeforeConstructor IgnoredField; +}; + +struct PositiveSeparateDefinition { + PositiveSeparateDefinition(); + int F; +}; + +PositiveSeparateDefinition::PositiveSeparateDefinition() {} +// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: constructor does not initialize these built-in/pointer fields: F +// CHECK-FIXES: PositiveSeparateDefinition::PositiveSeparateDefinition() : F() {} + +struct PositiveMixedFieldOrder { + PositiveMixedFieldOrder() : /* some comment */ J(0), L(0), M(0) {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these built-in/pointer fields: I, K, N + // CHECK-FIXES: PositiveMixedFieldOrder() : I(), /* some comment */ J(0), K(), L(0), M(0), N() {} + int I; + int J; + int K; + int L; + int M; + int N; +}; + +struct PositiveAfterBaseInitializer : public PositiveMixedFieldOrder { + PositiveAfterBaseInitializer() : PositiveMixedFieldOrder() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these built-in/pointer fields: F + // CHECK-FIXES: PositiveAfterBaseInitializer() : PositiveMixedFieldOrder(), F() {} + int F; +}; + +struct NegativeFieldInitialized { + int F; + + NegativeFieldInitialized() : F() {} +}; + +struct NegativeFieldInitializedInDefinition { + int F; + + NegativeFieldInitializedInDefinition(); +}; + +NegativeFieldInitializedInDefinition::NegativeFieldInitializedInDefinition() : F() {} + +struct NegativeInitializedInBody { + NegativeInitializedInBody() { I = 0; } + int I; +}; + + diff --git a/clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp b/clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp new file mode 100644 index 00000000000..62e7319ee89 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp @@ -0,0 +1,111 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-member-init %t + +struct PositiveFieldBeforeConstructor { + int F; + // CHECK-FIXES: int F{}; + PositiveFieldBeforeConstructor() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these built-in/pointer fields: F + // CHECK-FIXES: PositiveFieldBeforeConstructor() {} +}; + +struct PositiveFieldAfterConstructor { + PositiveFieldAfterConstructor() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these built-in/pointer fields: F, G + // CHECK-FIXES: PositiveFieldAfterConstructor() {} + int F; + // CHECK-FIXES: int F{}; + bool G /* with comment */; + // CHECK-FIXES: bool G{} /* with comment */; + PositiveFieldBeforeConstructor IgnoredField; +}; + +struct PositiveSeparateDefinition { + PositiveSeparateDefinition(); + int F; + // CHECK-FIXES: int F{}; +}; + +PositiveSeparateDefinition::PositiveSeparateDefinition() {} +// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: constructor does not initialize these built-in/pointer fields: F +// CHECK-FIXES: PositiveSeparateDefinition::PositiveSeparateDefinition() {} + +struct PositiveMixedFieldOrder { + PositiveMixedFieldOrder() : J(0) {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these built-in/pointer fields: I, K + // CHECK-FIXES: PositiveMixedFieldOrder() : J(0) {} + int I; + // CHECK-FIXES: int I{}; + int J; + int K; + // CHECK-FIXES: int K{}; +}; + +template <typename T> +struct Template { + Template() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these built-in/pointer fields: F + int F; + // CHECK-FIXES: int F{}; + T T1; + // CHECK-FIXES: T T1; +}; + +void instantiate() { + Template<int> TInt; +} + +struct NegativeFieldInitialized { + int F; + + NegativeFieldInitialized() : F() {} +}; + +struct NegativeFieldInitializedInDefinition { + int F; + + NegativeFieldInitializedInDefinition(); +}; +NegativeFieldInitializedInDefinition::NegativeFieldInitializedInDefinition() : F() {} + + +struct NegativeInClassInitialized { + int F = 0; + + NegativeInClassInitialized() {} +}; + +struct NegativeConstructorDelegated { + int F; + + NegativeConstructorDelegated(int F) : F(F) {} + NegativeConstructorDelegated() : NegativeConstructorDelegated(0) {} +}; + +struct NegativeInitializedInBody { + NegativeInitializedInBody() { I = 0; } + int I; +}; + +#define UNINITIALIZED_FIELD_IN_MACRO_BODY(FIELD) \ + struct UninitializedField##FIELD { \ + UninitializedField##FIELD() {} \ + int FIELD; \ + }; \ +// Ensure FIELD is not initialized since fixes inside of macros are disabled. +// CHECK-FIXES: int FIELD; + +UNINITIALIZED_FIELD_IN_MACRO_BODY(F); +// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: constructor does not initialize these built-in/pointer fields: F +UNINITIALIZED_FIELD_IN_MACRO_BODY(G); +// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: constructor does not initialize these built-in/pointer fields: G + +#define UNINITIALIZED_FIELD_IN_MACRO_ARGUMENT(ARGUMENT) \ + ARGUMENT \ + +UNINITIALIZED_FIELD_IN_MACRO_ARGUMENT(struct UninitializedFieldInMacroArg { + UninitializedFieldInMacroArg() {} + int Field; +}); +// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: constructor does not initialize these built-in/pointer fields: Field +// Ensure FIELD is not initialized since fixes inside of macros are disabled. +// CHECK-FIXES: int Field; |