Index: clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h =================================================================== --- clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h +++ clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h @@ -16,8 +16,12 @@ namespace tidy { namespace cppcoreguidelines { -/// \brief Checks that builtin or pointer fields are initialized by -/// user-defined constructors. +/// \brief Implements C++ Core Guidelines Type.6. +/// +/// Checks that every user-provided constructor value-initializes all class +/// members and base classes that would have undefined behavior otherwise. Also +/// check that any record types without user-provided default constructors are +/// value-initialized where used. /// /// Members initialized through function calls in the body of the constructor /// will result in false positives. @@ -25,15 +29,40 @@ /// 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 {}." +/// TODO: For classes with multiple constructors, make sure that we don't offer +/// multiple in-class initializer fixits for the same member. class ProTypeMemberInitCheck : public ClangTidyCheck { public: - ProTypeMemberInitCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + ProTypeMemberInitCheck(StringRef Name, ClangTidyContext *Context); void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + +private: + // Checks Type.6 part 1: + // Issue a diagnostic for any constructor of a non-trivially-constructible + // type that does not initialize all member variables. + // + // To fix: Write a data member initializer, or mention it in the member + // initializer list. + void checkMissingMemberInitializer(ASTContext &Context, + const CXXConstructorDecl *Ctor); + + // A subtle side effect of Type.6 part 2: + // Make sure to initialize trivially constructible base classes. + void checkMissingBaseClassInitializer(const ASTContext &Context, + const CXXConstructorDecl *Ctor); + + // Checks Type.6 part 2: + // Issue a diagnostic when constructing an object of a trivially constructible + // type without () or {} to initialize its members. + // + // To fix: Add () or {}. + void checkUninitializedTrivialType(const ASTContext &Context, + const VarDecl *Var); + + // Whether arrays need to be initialized or not. Default is false. + bool IgnoreArrays; }; } // namespace cppcoreguidelines Index: clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp +++ clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp @@ -9,12 +9,15 @@ #include "ProTypeMemberInitCheck.h" #include "../utils/LexerUtils.h" +#include "../utils/Matchers.h" +#include "../utils/TypeTraits.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 namespace clang::tidy::matchers; using llvm::SmallPtrSet; using llvm::SmallPtrSetImpl; @@ -24,16 +27,13 @@ namespace { -AST_MATCHER(CXXConstructorDecl, isUserProvided) { - return Node.isUserProvided(); -} - -static void -fieldsRequiringInit(const RecordDecl::field_range &Fields, - SmallPtrSetImpl &FieldsToInit) { +void fieldsRequiringInit(const RecordDecl::field_range &Fields, + ASTContext &Context, + SmallPtrSetImpl &FieldsToInit) { for (const FieldDecl *F : Fields) { QualType Type = F->getType(); - if (Type->isPointerType() || Type->isBuiltinType()) + if (!F->hasInClassInitializer() && + type_traits::isTriviallyDefaultConstructible(Type, Context)) FieldsToInit.insert(F); } } @@ -50,134 +50,259 @@ FieldDecls.erase(Match.getNodeAs("fieldDecl")); } -// Creates comma separated list of fields requiring initialization in order of +StringRef getName(const FieldDecl *Field) { return Field->getName(); } + +StringRef getName(const RecordDecl *Record) { + // Get the typedef name if this is a C-style anonymous struct and typedef. + if (const TypedefNameDecl *Typedef = Record->getTypedefNameForAnonDecl()) + return Typedef->getName(); + return Record->getName(); +} + +// Creates comma separated list of decls requiring initialization in order of // declaration. -std::string toCommaSeparatedString( - const RecordDecl::field_range &FieldRange, - const SmallPtrSetImpl &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 Fields; +template +std::string +toCommaSeparatedString(const R &OrderedDecls, + const SmallPtrSetImpl &DeclsToInit) { + SmallVector Names; + for (const T *Decl : OrderedDecls) { + if (DeclsToInit.count(Decl)) + Names.emplace_back(getName(Decl)); + } + return llvm::join(Names.begin(), Names.end(), ", "); +} + +SourceLocation getLocationForEndOfToken(const ASTContext &Context, + SourceLocation Location) { + return Lexer::getLocForEndOfToken(Location, 0, Context.getSourceManager(), + Context.getLangOpts()); +}; + +// There are 3 kinds of insertion placements: +enum class InitializerPlacement { + // 1. The fields are inserted after an existing CXXCtorInitializer stored in + // Where. This will be the case whenever there is a written initializer before + // the fields available. + After, + + // 2. The fields are inserted before the first existing initializer stored in + // Where. + Before, + + // 3. There are no written initializers and the fields will be inserted before + // the constructor's body creating a new initializer list including the ':'. + New +}; + +// An InitializerInsertion contains a list of fields and/or base classes to +// insert into the initializer list of a constructor. We use this to ensure +// proper absolute ordering according to the class declaration relative to the +// (perhaps improper) ordering in the existing initializer list, if any. +struct IntializerInsertion { + IntializerInsertion(InitializerPlacement Placement, + const CXXCtorInitializer *Where) + : Placement(Placement), Where(Where) {} 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()); + assert((Where != nullptr || Placement == InitializerPlacement::New) && + "Location should be relative to an existing initializer or this " + "insertion represents a new initializer list."); + SourceLocation Location; + switch (Placement) { + case InitializerPlacement::New: + Location = lexer_utils::getPreviousNonCommentToken( + Context, Constructor.getBody()->getLocStart()) + .getLocation(); + break; + case InitializerPlacement::Before: + Location = lexer_utils::getPreviousNonCommentToken( + Context, Where->getSourceRange().getBegin()) + .getLocation(); + break; + case InitializerPlacement::After: + Location = Where->getRParenLoc(); + break; + } + return getLocationForEndOfToken(Context, Location); } std::string codeToInsert() const { - assert(!Fields.empty() && "No fields to insert"); + assert(!Initializers.empty() && "No initializers 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] = ':'; + std::string joined = + llvm::join(Initializers.begin(), Initializers.end(), "(), "); + switch (Placement) { + case InitializerPlacement::New: + Stream << " : " << joined << "()"; + break; + case InitializerPlacement::Before: + Stream << " " << joined << "(),"; + break; + case InitializerPlacement::After: + Stream << ", " << joined << "()"; + break; + } return Code; } + + InitializerPlacement Placement; + const CXXCtorInitializer *Where; + SmallVector Initializers; }; -SmallVector computeInsertions( - const CXXConstructorDecl::init_const_range &Inits, - const RecordDecl::field_range &Fields, - const SmallPtrSetImpl &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 OrderedFields; - OrderedFields.push_back({LastWrittenNonMemberInit, nullptr, {}}); +// Convenience utility to get a RecordDecl from a QualType. +const RecordDecl *getCanonicalRecordDecl(const QualType &Type) { + if (const auto *RT = Type.getCanonicalType()->getAs()) + return RT->getDecl(); + return nullptr; +} - auto CurrentField = Fields.begin(); +template +SmallVector +computeInsertions(const CXXConstructorDecl::init_const_range &Inits, + const R &OrderedDecls, + const SmallPtrSetImpl &DeclsToInit) { + SmallVector Insertions; + Insertions.emplace_back(InitializerPlacement::New, nullptr); + + typename R::const_iterator Decl = std::begin(OrderedDecls); 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 (Init->isWritten()) { + if (Insertions.size() == 1) + Insertions.emplace_back(InitializerPlacement::Before, Init); + + // Gets either the field or base class being initialized by the provided + // initializer. + const auto *InitDecl = + Init->isMemberInitializer() + ? static_cast(Init->getMember()) + : Init->getBaseClass()->getAs()->getDecl(); + + // Add all fields between current field up until the next intializer. + for (; Decl != std::end(OrderedDecls) && *Decl != InitDecl; ++Decl) { + if (const T *D = dyn_cast(*Decl)) { + if (DeclsToInit.count(D) > 0) + Insertions.back().Initializers.emplace_back(getName(D)); + } } - // 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, {}}); + + Insertions.emplace_back(InitializerPlacement::After, Init); } } - // 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); + + // Add remaining decls that require initialization. + for (; Decl != std::end(OrderedDecls); ++Decl) { + if (const T *D = dyn_cast(*Decl)) { + if (DeclsToInit.count(D) > 0) + Insertions.back().Initializers.emplace_back(getName(D)); + } + } + return Insertions; +} + +// Gets the list of bases and members that could possibly be initialized, in +// order as they appear in the class declaration. +void getInitializationsInOrder(const CXXRecordDecl *ClassDecl, + SmallVectorImpl &Decls) { + Decls.clear(); + for (const auto &Base : ClassDecl->bases()) + Decls.emplace_back(getCanonicalRecordDecl(Base.getType())); + Decls.append(ClassDecl->fields().begin(), ClassDecl->fields().end()); +} + +template +void fixInitializerList(const ASTContext &Context, DiagnosticBuilder &Diag, + const CXXConstructorDecl *Ctor, + const SmallPtrSetImpl &DeclsToInit) { + // Do not propose fixes in macros since we cannot place them correctly. + if (Ctor->getLocStart().isMacroID()) + return; + + SmallVector OrderedDecls; + getInitializationsInOrder(Ctor->getParent(), OrderedDecls); + + for (const auto &Insertion : + computeInsertions(Ctor->inits(), OrderedDecls, DeclsToInit)) { + if (!Insertion.Initializers.empty()) + Diag << FixItHint::CreateInsertion(Insertion.getLocation(Context, *Ctor), + Insertion.codeToInsert()); + } +} + +template +void forEachField(const RecordDecl *Record, const T &Fields, + bool OneFieldPerUnion, Func &&Fn) { + for (const FieldDecl *F : Fields) { + if (F->isAnonymousStructOrUnion()) { + if (const RecordDecl *R = getCanonicalRecordDecl(F->getType())) + forEachField(R, R->fields(), OneFieldPerUnion, Fn); + } else { + Fn(F); + } + + if (OneFieldPerUnion && Record->isUnion()) + break; } - return OrderedFields; } } // namespace +ProTypeMemberInitCheck::ProTypeMemberInitCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + IgnoreArrays(Options.get("IgnoreArrays", false)) {} + void ProTypeMemberInitCheck::registerMatchers(MatchFinder *Finder) { - Finder->addMatcher(cxxConstructorDecl(isDefinition(), isUserProvided(), - unless(isInstantiated())) - .bind("ctor"), - this); + if (!getLangOpts().CPlusPlus) + return; + + auto IsUserProvidedNonDelegatingConstructor = + allOf(isUserProvided(), + unless(anyOf(isInstantiated(), isDelegatingConstructor()))); + auto IsNonTrivialDefaultConstructor = allOf( + isDefaultConstructor(), unless(isUserProvided()), + hasParent(cxxRecordDecl(unless(isTriviallyDefaultConstructible())))); + Finder->addMatcher( + cxxConstructorDecl(isDefinition(), + anyOf(IsUserProvidedNonDelegatingConstructor, + IsNonTrivialDefaultConstructor)) + .bind("ctor"), + this); + auto HasDefaultConstructor = hasInitializer( + cxxConstructExpr(unless(requiresZeroInitialization()), + hasDeclaration(cxxConstructorDecl( + isDefaultConstructor(), unless(isUserProvided()))))); + Finder->addMatcher( + varDecl(isDefinition(), HasDefaultConstructor, + hasType(recordDecl(has(fieldDecl()), + isTriviallyDefaultConstructible()))) + .bind("var"), + this); } void ProTypeMemberInitCheck::check(const MatchFinder::MatchResult &Result) { - const auto *Ctor = Result.Nodes.getNodeAs("ctor"); - const auto &MemberFields = Ctor->getParent()->fields(); + if (const auto *Ctor = Result.Nodes.getNodeAs("ctor")) { + checkMissingMemberInitializer(*Result.Context, Ctor); + checkMissingBaseClassInitializer(*Result.Context, Ctor); + } else if (const auto *Var = Result.Nodes.getNodeAs("var")) { + checkUninitializedTrivialType(*Result.Context, Var); + } +} + +void ProTypeMemberInitCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "IgnoreArrays", IgnoreArrays); +} + +void ProTypeMemberInitCheck::checkMissingMemberInitializer( + ASTContext &Context, const CXXConstructorDecl *Ctor) { + const CXXRecordDecl *ClassDecl = Ctor->getParent(); + bool IsUnion = ClassDecl->isUnion(); + + if (IsUnion && ClassDecl->hasInClassInitializer()) + return; // Skip declarations delayed by late template parsing without a body. const Stmt *Body = Ctor->getBody(); @@ -185,50 +310,113 @@ return; SmallPtrSet FieldsToInit; - fieldsRequiringInit(MemberFields, FieldsToInit); + fieldsRequiringInit(ClassDecl->fields(), Context, 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()); + for (const CXXCtorInitializer *Init : Ctor->inits()) { + // Remove any fields that were explicitly written in the initializer list + // or in-class. + if (Init->isAnyMemberInitializer() && Init->isWritten()) { + if (IsUnion) + return; // We can only initialize one member of a union. + FieldsToInit.erase(Init->getMember()); + } } - removeFieldsInitializedInBody(*Body, *Result.Context, FieldsToInit); + removeFieldsInitializedInBody(*Body, Context, FieldsToInit); - if (FieldsToInit.empty()) + // Collect all fields in order, both direct fields and indirect fields from + // anonmyous record types. + SmallVector OrderedFields; + forEachField(ClassDecl, ClassDecl->fields(), false, + [&](const FieldDecl *F) { OrderedFields.push_back(F); }); + + // Collect all the fields we need to initialize, including indirect fields. + SmallPtrSet AllFieldsToInit; + forEachField(ClassDecl, FieldsToInit, false, + [&](const FieldDecl *F) { AllFieldsToInit.insert(F); }); + if (AllFieldsToInit.empty()) return; DiagnosticBuilder Diag = diag(Ctor->getLocStart(), - "constructor does not initialize these built-in/pointer fields: %0") - << toCommaSeparatedString(MemberFields, FieldsToInit); + IsUnion + ? "union constructor should initialize one of these fields: %0" + : "constructor does not initialize these fields: %0") + << toCommaSeparatedString(OrderedFields, AllFieldsToInit); + // 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) { + + // Collect all fields but only suggest a fix for the first member of unions, + // as initializing more than one union member is an error. + SmallPtrSet FieldsToFix; + forEachField(ClassDecl, FieldsToInit, true, [&](const FieldDecl *F) { + // Don't suggest fixes for enums because we don't know a good default. + if (!F->getType()->isEnumeralType()) + FieldsToFix.insert(F); + }); + if (FieldsToFix.empty()) + return; + + // Use in-class initialization if possible. + if (Context.getLangOpts().CPlusPlus11) { + for (const FieldDecl *Field : FieldsToFix) { Diag << FixItHint::CreateInsertion( - Lexer::getLocForEndOfToken(Field->getSourceRange().getEnd(), 0, - Result.Context->getSourceManager(), - Result.Context->getLangOpts()), + getLocationForEndOfToken(Context, Field->getSourceRange().getEnd()), "{}"); } - return; + } else { + // Otherwise, rewrite the constructor's initializer list. + fixInitializerList(Context, Diag, Ctor, FieldsToFix); } - for (const auto &FieldsInsertion : - computeInsertions(Ctor->inits(), MemberFields, FieldsToInit)) { - if (!FieldsInsertion.Fields.empty()) - Diag << FixItHint::CreateInsertion( - FieldsInsertion.getLocation(*Result.Context, *Ctor), - FieldsInsertion.codeToInsert()); +} + +void ProTypeMemberInitCheck::checkMissingBaseClassInitializer( + const ASTContext &Context, const CXXConstructorDecl *Ctor) { + const CXXRecordDecl *ClassDecl = Ctor->getParent(); + + // Gather any base classes that need to be initialized. + SmallVector AllBases; + SmallPtrSet BasesToInit; + for (const CXXBaseSpecifier &Base : ClassDecl->bases()) { + if (const auto *BaseClassDecl = getCanonicalRecordDecl(Base.getType())) { + AllBases.emplace_back(BaseClassDecl); + if (!BaseClassDecl->field_empty() && + type_traits::isTriviallyDefaultConstructible(Base.getType(), Context)) + BasesToInit.insert(BaseClassDecl); + } } + + if (BasesToInit.empty()) + return; + + // Remove any bases that were explicitly written in the initializer list. + for (const CXXCtorInitializer *Init : Ctor->inits()) { + if (Init->isBaseInitializer() && Init->isWritten()) + BasesToInit.erase(Init->getBaseClass()->getAs()->getDecl()); + } + + if (BasesToInit.empty()) + return; + + DiagnosticBuilder Diag = + diag(Ctor->getLocStart(), + "constructor does not initialize these bases: %0") + << toCommaSeparatedString(AllBases, BasesToInit); + + fixInitializerList(Context, Diag, Ctor, BasesToInit); +} + +void ProTypeMemberInitCheck::checkUninitializedTrivialType( + const ASTContext &Context, const VarDecl *Var) { + DiagnosticBuilder Diag = + diag(Var->getLocStart(), "uninitialized record type: %0") << Var; + + Diag << FixItHint::CreateInsertion( + getLocationForEndOfToken(Context, Var->getSourceRange().getEnd()), + Context.getLangOpts().CPlusPlus11 ? "{}" : " = {}"); } } // namespace cppcoreguidelines Index: clang-tools-extra/trunk/clang-tidy/utils/Matchers.h =================================================================== --- clang-tools-extra/trunk/clang-tidy/utils/Matchers.h +++ clang-tools-extra/trunk/clang-tidy/utils/Matchers.h @@ -23,6 +23,11 @@ return IsExpensive && *IsExpensive; } +AST_MATCHER(RecordDecl, isTriviallyDefaultConstructible) { + return type_traits::recordIsTriviallyDefaultConstructible( + Node, Finder->getASTContext()); +} + } // namespace matchers } // namespace tidy } // namespace clang Index: clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.h =================================================================== --- clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.h +++ clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.h @@ -20,6 +20,13 @@ // \brief Returns true If \c Type is expensive to copy. llvm::Optional isExpensiveToCopy(QualType Type, ASTContext &Context); +// \brief Returns true If \c Type is trivially default constructible. +bool isTriviallyDefaultConstructible(QualType Type, const ASTContext &Context); + +// \brief Returns true If \c RecordDecl is trivially default constructible. +bool recordIsTriviallyDefaultConstructible(const RecordDecl &RecordDecl, + const ASTContext &Context); + } // type_traits } // namespace tidy } // namespace clang Index: clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp +++ clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp @@ -31,6 +31,81 @@ !classHasTrivialCopyAndDestroy(Type); } +bool recordIsTriviallyDefaultConstructible(const RecordDecl &RecordDecl, + const ASTContext &Context) { + const auto *ClassDecl = dyn_cast(&RecordDecl); + // Non-C++ records are always trivially constructible. + if (!ClassDecl) + return true; + // A class with a user-provided default constructor is not trivially + // constructible. + if (ClassDecl->hasUserProvidedDefaultConstructor()) + return false; + // A class is trivially constructible if it has a trivial default constructor. + if (ClassDecl->hasTrivialDefaultConstructor()) + return true; + + // If all its fields are trivially constructible. + for (const FieldDecl *Field : ClassDecl->fields()) { + if (!isTriviallyDefaultConstructible(Field->getType(), Context)) + return false; + } + // If all its direct bases are trivially constructible. + for (const CXXBaseSpecifier &Base : ClassDecl->bases()) { + if (!isTriviallyDefaultConstructible(Base.getType(), Context)) + return false; + } + + return true; +} + +// Based on QualType::isTrivial. +bool isTriviallyDefaultConstructible(QualType Type, const ASTContext &Context) { + if (Type.isNull()) + return false; + + if (Type->isArrayType()) + return isTriviallyDefaultConstructible(Context.getBaseElementType(Type), + Context); + + // Return false for incomplete types after skipping any incomplete array + // types which are expressly allowed by the standard and thus our API. + if (Type->isIncompleteType()) + return false; + + if (Context.getLangOpts().ObjCAutoRefCount) { + switch (Type.getObjCLifetime()) { + case Qualifiers::OCL_ExplicitNone: + return true; + + case Qualifiers::OCL_Strong: + case Qualifiers::OCL_Weak: + case Qualifiers::OCL_Autoreleasing: + return false; + + case Qualifiers::OCL_None: + if (Type->isObjCLifetimeType()) + return false; + break; + } + } + + QualType CanonicalType = Type.getCanonicalType(); + if (CanonicalType->isDependentType()) + return false; + + // As an extension, Clang treats vector types as Scalar types. + if (CanonicalType->isScalarType() || CanonicalType->isVectorType()) + return true; + + if (const auto *RT = CanonicalType->getAs()) { + return recordIsTriviallyDefaultConstructible(*RT->getDecl(), Context); + } + + // No other types can match. + return false; +} + } // type_traits } // 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 @@ -276,6 +276,22 @@ * `readability-uniqueptr-delete-release `_ +- Updated ``cppcoreguidelines-pro-member-type-member-init`` check + + This check now conforms to C++ Core Guidelines rule Type.6: Always Initialize + a Member Variable. The check examines every record type where construction + might result in an undefined memory state. These record types needing + initialization have at least one default-initialized built-in, pointer, + array or record type matching these criteria or a default-initialized + direct base class of this kind. + + The check has two complementary aspects: + 1. Ensure every constructor for a record type needing initialization + value-initializes all members and direct bases via a combination of + in-class initializers and the member initializer list. + 2. Value-initialize every non-member instance of a record type needing + initialization that lacks a user-provided default constructor, e.g. + a POD. Improvements to modularize -------------------------- Index: clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-pro-type-member-init.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-pro-type-member-init.rst +++ clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-pro-type-member-init.rst @@ -3,16 +3,35 @@ 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. +The check flags user-defined constructor definitions that do not +initialize all fields that would be left in an undefined state by +default construction, e.g. builtins, pointers and record types without +user-provided default constructors containing at least one such +type. If these fields aren't initialized, the constructor will leave +some of the 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. +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. It will also initialize any direct base classes that +need to be zeroed in the constructor initializer list. -This rule is part of the "Type safety" profile of the C++ Core Guidelines, see +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. + +The check also flags variables of record types without a user-provided +constructor that are not initialized. The suggested fix is to zero +initialize the variable via {} for C++11 and beyond or = {} for older +versions. + +IgnoreArrays option +------------------- + +For performance critical code, it may be important to not zero +fixed-size array members. If on, IgnoreArrays will not warn about +array members that are not zero-initialized during construction. +IgnoreArrays is false by default. + +This rule is part of the "Type safety" profile of the C++ Core +Guidelines, corresponding to rule Type.6. See https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Pro-type-memberinit. Index: clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init-cxx98.cpp =================================================================== --- clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init-cxx98.cpp +++ clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init-cxx98.cpp @@ -3,13 +3,13 @@ struct PositiveFieldBeforeConstructor { int F; PositiveFieldBeforeConstructor() /* some comment */ {} - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these built-in/pointer fields: F + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these 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-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: F, G, H // CHECK-FIXES: PositiveFieldAfterConstructor() : F(), G(), H() {} int F; bool G /* with comment */; @@ -23,12 +23,12 @@ }; PositiveSeparateDefinition::PositiveSeparateDefinition() {} -// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: constructor does not initialize these built-in/pointer fields: F +// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: constructor does not initialize these 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-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: I, K, N // CHECK-FIXES: PositiveMixedFieldOrder() : I(), /* some comment */ J(0), K(), L(0), M(0), N() {} int I; int J; @@ -40,7 +40,7 @@ struct PositiveAfterBaseInitializer : public PositiveMixedFieldOrder { PositiveAfterBaseInitializer() : PositiveMixedFieldOrder() {} - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these built-in/pointer fields: F + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: F // CHECK-FIXES: PositiveAfterBaseInitializer() : PositiveMixedFieldOrder(), F() {} int F; }; @@ -64,4 +64,32 @@ int I; }; +struct NegativeAggregateType { + int X; + int Y; + int Z; +}; + +struct NonTrivialType { + int X; + int Y; +}; + +struct PositiveUninitializedBaseOrdering : public NegativeAggregateType, + public NonTrivialType { + PositiveUninitializedBaseOrdering() : NegativeAggregateType(), NonTrivialType(), B() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: A + // CHECK-FIXES: PositiveUninitializedBaseOrdering() : NegativeAggregateType(), NonTrivialType(), A(), B() {} + + // This is somewhat pathological with the base class initializer at the end... + PositiveUninitializedBaseOrdering(int) : B(), NonTrivialType(), A() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these bases: NegativeAggregateType + // CHECK-FIXES: PositiveUninitializedBaseOrdering(int) : B(), NegativeAggregateType(), NonTrivialType(), A() {} + + PositiveUninitializedBaseOrdering(float) : NegativeAggregateType(), A() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these bases: NonTrivialType + // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: constructor does not initialize these fields: B + // CHECK-FIXES: PositiveUninitializedBaseOrdering(float) : NegativeAggregateType(), NonTrivialType(), A(), B() {} + int A, B; +}; Index: clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init-delayed.cpp =================================================================== --- clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init-delayed.cpp +++ clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init-delayed.cpp @@ -1,20 +1,20 @@ // RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-member-init %t -- -- -fdelayed-template-parsing -template +template struct PositiveFieldBeforeConstructor { - PositiveFieldBeforeConstructor() {} - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these built-in/pointer fields: F, G, H int F; bool G /* with comment */; int *H; + PositiveFieldBeforeConstructor() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: F, G, H }; // Explicit instantiation. template class PositiveFieldBeforeConstructor; -template +template struct PositiveFieldAfterConstructor { PositiveFieldAfterConstructor() {} - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these built-in/pointer fields: F, G, H + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: F, G, H int F; bool G /* with comment */; int *H; @@ -25,7 +25,7 @@ // This declaration isn't used and won't be parsed 'delayed-template-parsing'. // The body of the declaration is 'null' and may cause crash if not handled // properly by checkers. -template +template struct UnusedDelayedConstructor { UnusedDelayedConstructor() {} int F; Index: clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp =================================================================== --- clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp +++ clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp @@ -4,13 +4,13 @@ int F; // CHECK-FIXES: int F{}; PositiveFieldBeforeConstructor() {} - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these built-in/pointer fields: F + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these 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-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: F, G // CHECK-FIXES: PositiveFieldAfterConstructor() {} int F; // CHECK-FIXES: int F{}; @@ -26,12 +26,12 @@ }; PositiveSeparateDefinition::PositiveSeparateDefinition() {} -// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: constructor does not initialize these built-in/pointer fields: F +// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: constructor does not initialize these 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-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: I, K // CHECK-FIXES: PositiveMixedFieldOrder() : J(0) {} int I; // CHECK-FIXES: int I{}; @@ -43,7 +43,7 @@ template struct Template { Template() {} - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these built-in/pointer fields: F + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: F int F; // CHECK-FIXES: int F{}; T T1; @@ -67,7 +67,6 @@ }; NegativeFieldInitializedInDefinition::NegativeFieldInitializedInDefinition() : F() {} - struct NegativeInClassInitialized { int F = 0; @@ -87,25 +86,248 @@ }; #define UNINITIALIZED_FIELD_IN_MACRO_BODY(FIELD) \ - struct UninitializedField##FIELD { \ - UninitializedField##FIELD() {} \ - int 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 +// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: constructor does not initialize these fields: F UNINITIALIZED_FIELD_IN_MACRO_BODY(G); -// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: constructor does not initialize these built-in/pointer fields: G +// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: constructor does not initialize these fields: G #define UNINITIALIZED_FIELD_IN_MACRO_ARGUMENT(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 +// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: constructor does not initialize these fields: Field // Ensure FIELD is not initialized since fixes inside of macros are disabled. // CHECK-FIXES: int Field; + +struct NegativeAggregateType { + int X; + int Y; + int Z; +}; + +struct PositiveTrivialType { + PositiveTrivialType() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: F + + NegativeAggregateType F; + // CHECK-FIXES: NegativeAggregateType F{}; +}; + +struct NegativeNonTrivialType { + PositiveTrivialType F; +}; + +static void PositiveUninitializedTrivialType() { + NegativeAggregateType X; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: uninitialized record type: 'X' + // CHECK-FIXES: NegativeAggregateType X{}; + + NegativeAggregateType A[10]; // Don't warn because this isn't an object type. +} + +static void NegativeInitializedTrivialType() { + NegativeAggregateType X{}; + NegativeAggregateType Y = {}; + NegativeAggregateType Z = NegativeAggregateType(); + NegativeAggregateType A[10]{}; + NegativeAggregateType B[10] = {}; + int C; // No need to initialize this because we don't have a constructor. + int D[8]; + NegativeAggregateType E = {0, 1, 2}; + NegativeAggregateType F({}); +} + +struct NonTrivialType { + NonTrivialType() = default; + NonTrivialType(const NonTrivialType &RHS) : X(RHS.X), Y(RHS.Y) {} + + int X; + int Y; +}; + +static void PositiveNonTrivialTypeWithCopyConstructor() { + NonTrivialType T; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: uninitialized record type: 'T' + // CHECK-FIXES: NonTrivialType T{}; + + NonTrivialType A[8]; + // Don't warn because this isn't an object type +} + +struct ComplexNonTrivialType { + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: constructor does not initialize these fields: Y + NegativeFieldInitialized X; + int Y; + // CHECK-FIXES: int Y{}; +}; + +static void PositiveComplexNonTrivialType() { + ComplexNonTrivialType T; +} + +struct PositiveStaticMember { + static NonTrivialType X; + static NonTrivialType Y; + static constexpr NonTrivialType Z{}; +}; + +NonTrivialType PositiveStaticMember::X; +// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: uninitialized record type: 'X' +// CHECK-FIXES: NonTrivialType PositiveStaticMember::X{}; + +NonTrivialType PositiveStaticMember::Y{}; + +struct PositiveMultipleConstructors { + PositiveMultipleConstructors() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: A, B + + PositiveMultipleConstructors(int) {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: A, B + + PositiveMultipleConstructors(const PositiveMultipleConstructors &) {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: A, B + + // FIXME: The fix-its here collide providing an erroneous fix + int A, B; + // CHECK-FIXES: int A{}{}{}, B{}{}{}; +}; + +typedef struct { + int Member; +} CStyleStruct; + +struct PositiveUninitializedBase : public NegativeAggregateType, CStyleStruct { + PositiveUninitializedBase() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these bases: NegativeAggregateType, CStyleStruct + // CHECK-FIXES: PositiveUninitializedBase() : NegativeAggregateType(), CStyleStruct() {} +}; + +struct PositiveUninitializedBaseOrdering : public NegativeAggregateType, + public NonTrivialType { + PositiveUninitializedBaseOrdering() : B() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these bases: NegativeAggregateType, NonTrivialType + // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: constructor does not initialize these fields: A + // CHECK-FIXES: PositiveUninitializedBaseOrdering() : NegativeAggregateType(), NonTrivialType(), B() {} + + // This is somewhat pathological with the base class initializer at the end... + PositiveUninitializedBaseOrdering(int) : B(), NonTrivialType(), A() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these bases: NegativeAggregateType + // CHECK-FIXES: PositiveUninitializedBaseOrdering(int) : B(), NegativeAggregateType(), NonTrivialType(), A() {} + + PositiveUninitializedBaseOrdering(float) : B(), NegativeAggregateType(), A() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these bases: NonTrivialType + // CHECK-FIXES: PositiveUninitializedBaseOrdering(float) : B(), NegativeAggregateType(), NonTrivialType(), A() {} + + int A, B; + // CHECK-FIXES: int A{}, B; +}; + +// We shouldn't need to initialize anything because PositiveUninitializedBase +// has a user-defined constructor. +struct NegativeUninitializedBase : public PositiveUninitializedBase { + NegativeUninitializedBase() {} +}; + +struct InheritedAggregate : public NegativeAggregateType { + int F; +}; + +static InheritedAggregate PositiveGlobal; +// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: uninitialized record type: 'PositiveGlobal' +// CHECK-FIXES: InheritedAggregate PositiveGlobal{}; + +enum TestEnum { + A, + B, + C +}; + +enum class TestScopedEnum { + A, + B, + C +}; + +struct PositiveEnumType { + PositiveEnumType() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: X, Y + // No proposed fixes, as we don't know whether value initialization for these + // enums really makes sense. + + TestEnum X; + TestScopedEnum Y; +}; + +extern "C" { +struct NegativeCStruct { + int X, Y, Z; +}; + +static void PositiveCStructVariable() { + NegativeCStruct X; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: uninitialized record type: 'X' + // CHECK-FIXES: NegativeCStruct X{}; +} +} + +union NegativeUnionInClass { + NegativeUnionInClass() {} // No message as a union can only initialize one member. + int X = 0; + float Y; +}; + +union PositiveUnion { + PositiveUnion() : X() {} // No message as a union can only initialize one member. + PositiveUnion(int) {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: union constructor should initialize one of these fields: X, Y + + int X; + // CHECK-FIXES: int X{}; + + // Make sure we don't give Y an initializer. + float Y; + // CHECK-FIXES-NOT: float Y{}; +}; + +struct PositiveAnonymousUnionAndStruct { + PositiveAnonymousUnionAndStruct() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: A, B, Y, Z, C, D, E, F, X + + union { + int A; + // CHECK-FIXES: int A{}; + short B; + }; + + struct { + int Y; + // CHECK-FIXES: int Y{}; + char *Z; + // CHECK-FIXES: char *Z{}; + + struct { + short C; + // CHECK-FIXES: short C{}; + double D; + // CHECK-FIXES: double D{}; + }; + + union { + long E; + // CHECK-FIXES: long E{}; + float F; + }; + }; + int X; + // CHECK-FIXES: int X{}; +};