Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -21,6 +21,7 @@ SwappedArgumentsCheck.cpp ThrowByValueCatchByReferenceCheck.cpp UndelegatedConstructor.cpp + UninitializedFieldCheck.cpp UnusedAliasDeclsCheck.cpp UnusedParametersCheck.cpp UnusedRAIICheck.cpp Index: clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tidy/misc/MiscTidyModule.cpp +++ clang-tidy/misc/MiscTidyModule.cpp @@ -29,6 +29,7 @@ #include "SwappedArgumentsCheck.h" #include "ThrowByValueCatchByReferenceCheck.h" #include "UndelegatedConstructor.h" +#include "UninitializedFieldCheck.h" #include "UniqueptrResetReleaseCheck.h" #include "UnusedAliasDeclsCheck.h" #include "UnusedParametersCheck.h" @@ -77,6 +78,8 @@ "misc-throw-by-value-catch-by-reference"); CheckFactories.registerCheck( "misc-undelegated-constructor"); + CheckFactories.registerCheck( + "misc-uninitialized-field"); CheckFactories.registerCheck( "misc-uniqueptr-reset-release"); CheckFactories.registerCheck( Index: clang-tidy/misc/UninitializedFieldCheck.h =================================================================== --- /dev/null +++ clang-tidy/misc/UninitializedFieldCheck.h @@ -0,0 +1,40 @@ +//===--- UninitializedFieldCheck.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_UNINITIALIZED_FIELD_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNINITIALIZED_FIELD_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { + +/// \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/misc-uninitialized-field.html +/// TODO: See if 'fixes' for false positives are optimized away by the +/// compiler. +class UninitializedFieldCheck : public ClangTidyCheck { +public: + UninitializedFieldCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNINITIALIZED_FIELD_H + Index: clang-tidy/misc/UninitializedFieldCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/misc/UninitializedFieldCheck.cpp @@ -0,0 +1,222 @@ +//===--- UninitializedFieldCheck.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 "UninitializedFieldCheck.h" +#include "../utils/LexerUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include + +#include + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { + +namespace { + +static bool fieldRequiresInit(const FieldDecl *F) { + return F->getType()->isPointerType() || F->getType()->isBuiltinType(); +} + +void removeFieldsInitializedInBody( + const Stmt &Stmt, ASTContext &Context, + std::unordered_set &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")); +} + +// Creates comma separated list of fields requiring initialization in order of +// declaration. +std::string toCommaSeparatedString( + const RecordDecl::field_range &FieldRange, + const std::unordered_set &FieldsRequiringInit) { + std::string List; + llvm::raw_string_ostream Stream(List); + size_t AddedFields = 0; + for (const FieldDecl *Field : FieldRange) { + if (FieldsRequiringInit.find(Field) != FieldsRequiringInit.end()) { + 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; + std::vector Fields; + + SourceLocation getLocation(const ASTContext &Context, + const CXXConstructorDecl &Constructor) const { + if (InitializerBefore != nullptr) + return InitializerBefore->getRParenLoc().getLocWithOffset(1); + auto StartLocation = InitializerAfter != nullptr + ? InitializerAfter->getSourceRange().getBegin() + : Constructor.getBody()->getLocStart(); + auto Token = + lexer_utils::getPreviousNonCommentToken(Context, StartLocation); + return Token.getLocation().getLocWithOffset(Token.getLength()); + } + + 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() << "()"; + } + } + Code = Stream.str(); + // The initializer list is created, replace leading comma with colon. + if (InitializerBefore == nullptr && InitializerAfter == nullptr) { + Code[1] = ':'; + } + return Code; + } +}; + +std::vector computeInsertions( + const CXXConstructorDecl::init_const_range &Inits, + const RecordDecl::field_range &Fields, + const std::unordered_set &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; + } + std::vector 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.find(*CurrentField) != + FieldsRequiringInit.end()) + 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.find(*CurrentField) != FieldsRequiringInit.end()) + OrderedFields.back().Fields.push_back(*CurrentField); + } + return OrderedFields; +} + +} // namespace + +void UninitializedFieldCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher(cxxConstructorDecl(isDefinition()).bind("ctor"), this); +} + +void UninitializedFieldCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Ctor = Result.Nodes.getNodeAs("ctor"); + if (!Ctor->isUserProvided()) + return; + + auto ParentClass = Ctor->getParent(); + const auto &MemberFields = ParentClass->fields(); + + std::unordered_set FieldsToInit; + std::copy_if(MemberFields.begin(), MemberFields.end(), + std::inserter(FieldsToInit, FieldsToInit.end()), + &fieldRequiresInit); + if (FieldsToInit.empty()) + return; + + for (CXXCtorInitializer *Init : Ctor->inits()) { + if (Init->isDelegatingInitializer()) + return; + if (!Init->isMemberInitializer()) + continue; + const FieldDecl *MemberField = Init->getMember(); + FieldsToInit.erase(MemberField); + } + removeFieldsInitializedInBody(*Ctor->getBody(), *Result.Context, + FieldsToInit); + + if (FieldsToInit.empty()) + return; + + // For C+11 use in-class initialization which covers all future constructors + // as well. + if (Result.Context->getLangOpts().CPlusPlus11) { + DiagnosticBuilder builder = + diag( + Ctor->getLocStart(), + "constructor does not initialize these built-in/pointer fields: %0") + << toCommaSeparatedString(MemberFields, FieldsToInit); + + for (const auto *Field : FieldsToInit) + builder << FixItHint::CreateInsertion( + Field->getSourceRange().getEnd().getLocWithOffset( + Field->getName().size()), + "{}"); + return; + } + + DiagnosticBuilder builder = + diag(Ctor->getLocStart(), + "constructor does not initialize these built-in/pointer fields: %0") + << toCommaSeparatedString(MemberFields, FieldsToInit); + for (const auto &FieldsInsertion : + computeInsertions(Ctor->inits(), MemberFields, FieldsToInit)) + if (!FieldsInsertion.Fields.empty()) + builder << FixItHint::CreateInsertion( + FieldsInsertion.getLocation(*Result.Context, *Ctor), + FieldsInsertion.codeToInsert()); +} + +} // namespace tidy +} // namespace clang Index: docs/clang-tidy/checks/misc-uninitialized-field.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/misc-uninitialized-field.rst @@ -0,0 +1,16 @@ +.. title:: clang-tidy - misc-uninitialized-field + +misc-uninitialized-field +========================== + + +The check flags user-defined constructor definitions that don't 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. Index: test/clang-tidy/misc-uninitialized-field-cxx98.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-uninitialized-field-cxx98.cpp @@ -0,0 +1,65 @@ +// RUN: %check_clang_tidy %s misc-uninitialized-field %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; +}; Index: test/clang-tidy/misc-uninitialized-field.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-uninitialized-field.cpp @@ -0,0 +1,69 @@ +// RUN: %check_clang_tidy %s misc-uninitialized-field %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 +}; + +struct PositiveFieldAfterConstructor { + PositiveFieldAfterConstructor() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these built-in/pointer fields: F, G + 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 + +struct PositiveMixedFieldOrder { + PositiveMixedFieldOrder() : J(0) {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these built-in/pointer fields: I, K + int I; + // CHECK-FIXES: int I{}; + int J; + int K; + // CHECK-FIXES: int K{}; +}; + +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; +};