[clang-tools-extra] r283224 - Fix some false-positives with cppcoreguidelines-pro-type-member-init. Handle classes with default constructors that are defaulted or are not present in the AST.
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 4 07:48:05 PDT 2016
Author: aaronballman
Date: Tue Oct 4 09:48:05 2016
New Revision: 283224
URL: http://llvm.org/viewvc/llvm-project?rev=283224&view=rev
Log:
Fix some false-positives with cppcoreguidelines-pro-type-member-init. Handle classes with default constructors that are defaulted or are not present in the AST.
Classes with virtual methods or virtual bases are not trivially default constructible, so their members and bases need to be initialized.
Patch by Malcolm Parsons.
Modified:
clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h
clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp
Modified: clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp?rev=283224&r1=283223&r2=283224&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp Tue Oct 4 09:48:05 2016
@@ -32,17 +32,17 @@ namespace {
// the record type (or indirect field) is a union, forEachField will stop after
// the first field.
template <typename T, typename Func>
-void forEachField(const RecordDecl *Record, const T &Fields,
+void forEachField(const RecordDecl &Record, const T &Fields,
bool OneFieldPerUnion, Func &&Fn) {
for (const FieldDecl *F : Fields) {
if (F->isAnonymousStructOrUnion()) {
if (const CXXRecordDecl *R = F->getType()->getAsCXXRecordDecl())
- forEachField(R, R->fields(), OneFieldPerUnion, Fn);
+ forEachField(*R, R->fields(), OneFieldPerUnion, Fn);
} else {
Fn(F);
}
- if (OneFieldPerUnion && Record->isUnion())
+ if (OneFieldPerUnion && Record.isUnion())
break;
}
}
@@ -214,16 +214,16 @@ computeInsertions(const CXXConstructorDe
// 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,
+void getInitializationsInOrder(const CXXRecordDecl &ClassDecl,
SmallVectorImpl<const NamedDecl *> &Decls) {
Decls.clear();
- for (const auto &Base : ClassDecl->bases()) {
+ for (const auto &Base : ClassDecl.bases()) {
// Decl may be null if the base class is a template parameter.
if (const NamedDecl *Decl = getCanonicalRecordDecl(Base.getType())) {
Decls.emplace_back(Decl);
}
}
- forEachField(ClassDecl, ClassDecl->fields(), false,
+ forEachField(ClassDecl, ClassDecl.fields(), false,
[&](const FieldDecl *F) { Decls.push_back(F); });
}
@@ -236,7 +236,7 @@ void fixInitializerList(const ASTContext
return;
SmallVector<const NamedDecl *, 16> OrderedDecls;
- getInitializationsInOrder(Ctor->getParent(), OrderedDecls);
+ getInitializationsInOrder(*Ctor->getParent(), OrderedDecls);
for (const auto &Insertion :
computeInsertions(Ctor->inits(), OrderedDecls, DeclsToInit)) {
@@ -269,6 +269,19 @@ void ProTypeMemberInitCheck::registerMat
IsNonTrivialDefaultConstructor))
.bind("ctor"),
this);
+
+ // Match classes with a default constructor that is defaulted or is not in the
+ // AST.
+ Finder->addMatcher(
+ cxxRecordDecl(
+ isDefinition(), unless(isInstantiated()),
+ anyOf(has(cxxConstructorDecl(isDefaultConstructor(), isDefaulted(),
+ unless(isImplicit()))),
+ unless(has(cxxConstructorDecl()))),
+ unless(isTriviallyDefaultConstructible()))
+ .bind("record"),
+ this);
+
auto HasDefaultConstructor = hasInitializer(
cxxConstructExpr(unless(requiresZeroInitialization()),
hasDeclaration(cxxConstructorDecl(
@@ -287,8 +300,14 @@ void ProTypeMemberInitCheck::check(const
// Skip declarations delayed by late template parsing without a body.
if (!Ctor->getBody())
return;
- checkMissingMemberInitializer(*Result.Context, Ctor);
- checkMissingBaseClassInitializer(*Result.Context, Ctor);
+ checkMissingMemberInitializer(*Result.Context, *Ctor->getParent(), Ctor);
+ checkMissingBaseClassInitializer(*Result.Context, *Ctor->getParent(), Ctor);
+ } else if (const auto *Record =
+ Result.Nodes.getNodeAs<CXXRecordDecl>("record")) {
+ assert(Record->hasDefaultConstructor() &&
+ "Matched record should have a default constructor");
+ checkMissingMemberInitializer(*Result.Context, *Record, nullptr);
+ checkMissingBaseClassInitializer(*Result.Context, *Record, nullptr);
} else if (const auto *Var = Result.Nodes.getNodeAs<VarDecl>("var")) {
checkUninitializedTrivialType(*Result.Context, Var);
}
@@ -299,16 +318,16 @@ void ProTypeMemberInitCheck::storeOption
}
void ProTypeMemberInitCheck::checkMissingMemberInitializer(
- ASTContext &Context, const CXXConstructorDecl *Ctor) {
- const CXXRecordDecl *ClassDecl = Ctor->getParent();
- bool IsUnion = ClassDecl->isUnion();
+ ASTContext &Context, const CXXRecordDecl &ClassDecl,
+ const CXXConstructorDecl *Ctor) {
+ bool IsUnion = ClassDecl.isUnion();
- if (IsUnion && ClassDecl->hasInClassInitializer())
+ if (IsUnion && ClassDecl.hasInClassInitializer())
return;
// Gather all fields (direct and indirect) that need to be initialized.
SmallPtrSet<const FieldDecl *, 16> FieldsToInit;
- forEachField(ClassDecl, ClassDecl->fields(), false, [&](const FieldDecl *F) {
+ forEachField(ClassDecl, ClassDecl.fields(), false, [&](const FieldDecl *F) {
if (!F->hasInClassInitializer() &&
utils::type_traits::isTriviallyDefaultConstructible(F->getType(),
Context))
@@ -317,21 +336,23 @@ void ProTypeMemberInitCheck::checkMissin
if (FieldsToInit.empty())
return;
- 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->getAnyMember());
+ if (Ctor) {
+ 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->getAnyMember());
+ }
}
+ removeFieldsInitializedInBody(*Ctor->getBody(), Context, FieldsToInit);
}
- removeFieldsInitializedInBody(*Ctor->getBody(), Context, FieldsToInit);
// Collect all fields in order, both direct fields and indirect fields from
// anonmyous record types.
SmallVector<const FieldDecl *, 16> OrderedFields;
- forEachField(ClassDecl, ClassDecl->fields(), false,
+ forEachField(ClassDecl, ClassDecl.fields(), false,
[&](const FieldDecl *F) { OrderedFields.push_back(F); });
// Collect all the fields we need to initialize, including indirect fields.
@@ -342,14 +363,15 @@ void ProTypeMemberInitCheck::checkMissin
return;
DiagnosticBuilder Diag =
- diag(Ctor->getLocStart(),
+ diag(Ctor ? Ctor->getLocStart() : ClassDecl.getLocation(),
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())
+ // Do not propose fixes for constructors in macros since we cannot place them
+ // correctly.
+ if (Ctor && Ctor->getLocStart().isMacroID())
return;
// Collect all fields but only suggest a fix for the first member of unions,
@@ -370,20 +392,20 @@ void ProTypeMemberInitCheck::checkMissin
getLocationForEndOfToken(Context, Field->getSourceRange().getEnd()),
"{}");
}
- } else {
+ } else if (Ctor) {
// Otherwise, rewrite the constructor's initializer list.
fixInitializerList(Context, Diag, Ctor, FieldsToFix);
}
}
void ProTypeMemberInitCheck::checkMissingBaseClassInitializer(
- const ASTContext &Context, const CXXConstructorDecl *Ctor) {
- const CXXRecordDecl *ClassDecl = Ctor->getParent();
+ const ASTContext &Context, const CXXRecordDecl &ClassDecl,
+ const CXXConstructorDecl *Ctor) {
// Gather any base classes that need to be initialized.
SmallVector<const RecordDecl *, 4> AllBases;
SmallPtrSet<const RecordDecl *, 4> BasesToInit;
- for (const CXXBaseSpecifier &Base : ClassDecl->bases()) {
+ for (const CXXBaseSpecifier &Base : ClassDecl.bases()) {
if (const auto *BaseClassDecl = getCanonicalRecordDecl(Base.getType())) {
AllBases.emplace_back(BaseClassDecl);
if (!BaseClassDecl->field_empty() &&
@@ -397,20 +419,23 @@ void ProTypeMemberInitCheck::checkMissin
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()->getAsCXXRecordDecl());
+ if (Ctor) {
+ for (const CXXCtorInitializer *Init : Ctor->inits()) {
+ if (Init->isBaseInitializer() && Init->isWritten())
+ BasesToInit.erase(Init->getBaseClass()->getAsCXXRecordDecl());
+ }
}
if (BasesToInit.empty())
return;
DiagnosticBuilder Diag =
- diag(Ctor->getLocStart(),
+ diag(Ctor ? Ctor->getLocStart() : ClassDecl.getLocation(),
"constructor does not initialize these bases: %0")
<< toCommaSeparatedString(AllBases, BasesToInit);
- fixInitializerList(Context, Diag, Ctor, BasesToInit);
+ if (Ctor)
+ fixInitializerList(Context, Diag, Ctor, BasesToInit);
}
void ProTypeMemberInitCheck::checkUninitializedTrivialType(
Modified: clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h?rev=283224&r1=283223&r2=283224&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h (original)
+++ clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h Tue Oct 4 09:48:05 2016
@@ -46,11 +46,13 @@ private:
// To fix: Write a data member initializer, or mention it in the member
// initializer list.
void checkMissingMemberInitializer(ASTContext &Context,
+ const CXXRecordDecl &ClassDecl,
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 CXXRecordDecl &ClassDecl,
const CXXConstructorDecl *Ctor);
// Checks Type.6 part 2:
Modified: clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp?rev=283224&r1=283223&r2=283224&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp Tue Oct 4 09:48:05 2016
@@ -58,6 +58,9 @@ bool recordIsTriviallyDefaultConstructib
// constructible.
if (ClassDecl->hasUserProvidedDefaultConstructor())
return false;
+ // A polymorphic class is not trivially constructible
+ if (ClassDecl->isPolymorphic())
+ return false;
// A class is trivially constructible if it has a trivial default constructor.
if (ClassDecl->hasTrivialDefaultConstructor())
return true;
@@ -73,6 +76,8 @@ bool recordIsTriviallyDefaultConstructib
for (const CXXBaseSpecifier &Base : ClassDecl->bases()) {
if (!isTriviallyDefaultConstructible(Base.getType(), Context))
return false;
+ if (Base.isVirtual())
+ return false;
}
return true;
Modified: clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp?rev=283224&r1=283223&r2=283224&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp Tue Oct 4 09:48:05 2016
@@ -377,3 +377,49 @@ void Bug30487()
{
NegativeInClassInitializedDefaulted s;
}
+
+struct PositiveVirtualMethod {
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: constructor does not initialize these fields: F
+ int F;
+ // CHECK-FIXES: int F{};
+ virtual int f() = 0;
+};
+
+struct PositiveVirtualDestructor {
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: constructor does not initialize these fields: F
+ PositiveVirtualDestructor() = default;
+ int F;
+ // CHECK-FIXES: int F{};
+ virtual ~PositiveVirtualDestructor() {}
+};
+
+struct PositiveVirtualBase : public virtual NegativeAggregateType {
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: constructor does not initialize these bases: NegativeAggregateType
+ // CHECK-MESSAGES: :[[@LINE-2]]:8: warning: constructor does not initialize these fields: F
+ int F;
+ // CHECK-FIXES: int F{};
+};
+
+template <typename T>
+struct PositiveTemplateVirtualDestructor {
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: constructor does not initialize these fields: F
+ T Val;
+ int F;
+ // CHECK-FIXES: int F{};
+ virtual ~PositiveTemplateVirtualDestructor() = default;
+};
+
+template struct PositiveTemplateVirtualDestructor<int>;
+
+#define UNINITIALIZED_FIELD_IN_MACRO_BODY_VIRTUAL(FIELD) \
+ struct UninitializedFieldVirtual##FIELD { \
+ int FIELD; \
+ virtual ~UninitializedFieldVirtual##FIELD() {} \
+ }; \
+// Ensure FIELD is not initialized since fixes inside of macros are disabled.
+// CHECK-FIXES: int FIELD;
+
+UNINITIALIZED_FIELD_IN_MACRO_BODY_VIRTUAL(F);
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: constructor does not initialize these fields: F
+UNINITIALIZED_FIELD_IN_MACRO_BODY_VIRTUAL(G);
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: constructor does not initialize these fields: G
More information about the cfe-commits
mailing list