Fixed cppcoreguidelines-pro-type-member-init when checking records with indirect fields

Summary:
Fixed a crash in cppcoreguidelines-pro-type-member-init when checking record types with indirect fields pre-C++11.
Fixed handling of indirect fields so they are properly checked and suggested fixes are proposed.

Patch by Michael Miller!

Reviewers: aaron.ballman, alexfh, hokein

Subscribers: cfe-commits

Differential Revision: http://reviews.llvm.org/D19993

llvm-svn: 269024

clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp

@@ -27,16 +27,23 @@ namespace cppcoreguidelines {
 
 namespace {
 
-void fieldsRequiringInit(const RecordDecl::field_range &Fields,
-                         ASTContext &Context,
-                         SmallPtrSetImpl<const FieldDecl *> &FieldsToInit) {
+// Iterate over all the fields in a record type, both direct and indirect (e.g.
+// if the record contains an anonmyous struct). If OneFieldPerUnion is true and
+// 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,
+                  bool OneFieldPerUnion, Func &&Fn) {
   for (const FieldDecl *F : Fields) {
-    if (F->hasInClassInitializer())
-      continue;
-    QualType Type = F->getType();
-    if (!F->hasInClassInitializer() &&
-        utils::type_traits::isTriviallyDefaultConstructible(Type, Context))
-      FieldsToInit.insert(F);
+    if (F->isAnonymousStructOrUnion()) {
+      if (const CXXRecordDecl *R = F->getType()->getAsCXXRecordDecl())
+        forEachField(R, R->fields(), OneFieldPerUnion, Fn);
+    } else {
+      Fn(F);
+    }
+
+    if (OneFieldPerUnion && Record->isUnion())
+      break;
   }
 }
 
@@ -179,8 +186,8 @@ computeInsertions(const CXXConstructorDecl::init_const_range &Inits,
       // Gets either the field or base class being initialized by the provided
       // initializer.
       const auto *InitDecl =
-          Init->isMemberInitializer()
-              ? static_cast<const NamedDecl *>(Init->getMember())
+          Init->isAnyMemberInitializer()
+              ? static_cast<const NamedDecl *>(Init->getAnyMember())
               : Init->getBaseClass()->getAsCXXRecordDecl();
 
       // Add all fields between current field up until the next intializer.
@@ -216,7 +223,8 @@ void getInitializationsInOrder(const CXXRecordDecl *ClassDecl,
       Decls.emplace_back(Decl);
     }
   }
-  Decls.append(ClassDecl->fields().begin(), ClassDecl->fields().end());
+  forEachField(ClassDecl, ClassDecl->fields(), false,
+               [&](const FieldDecl *F) { Decls.push_back(F); });
 }
 
 template <typename T>
@@ -238,22 +246,6 @@ void fixInitializerList(const ASTContext &Context, DiagnosticBuilder &Diag,
   }
 }
 
-template <typename T, typename Func>
-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;
-  }
-}
-
 } // anonymous namespace
 
 ProTypeMemberInitCheck::ProTypeMemberInitCheck(StringRef Name,
@@ -314,8 +306,14 @@ void ProTypeMemberInitCheck::checkMissingMemberInitializer(
   if (IsUnion && ClassDecl->hasInClassInitializer())
     return;
 
+  // Gather all fields (direct and indirect) that need to be initialized.
   SmallPtrSet<const FieldDecl *, 16> FieldsToInit;
-  fieldsRequiringInit(ClassDecl->fields(), Context, FieldsToInit);
+  forEachField(ClassDecl, ClassDecl->fields(), false, [&](const FieldDecl *F) {
+    if (!F->hasInClassInitializer() &&
+        utils::type_traits::isTriviallyDefaultConstructible(F->getType(),
+                                                            Context))
+      FieldsToInit.insert(F);
+  });
   if (FieldsToInit.empty())
     return;
 
@@ -325,7 +323,7 @@ void ProTypeMemberInitCheck::checkMissingMemberInitializer(
     if (Init->isAnyMemberInitializer() && Init->isWritten()) {
       if (IsUnion)
         return; // We can only initialize one member of a union.
-      FieldsToInit.erase(Init->getMember());
+      FieldsToInit.erase(Init->getAnyMember());
     }
   }
   removeFieldsInitializedInBody(*Ctor->getBody(), Context, FieldsToInit);
@@ -389,8 +387,8 @@ void ProTypeMemberInitCheck::checkMissingBaseClassInitializer(
     if (const auto *BaseClassDecl = getCanonicalRecordDecl(Base.getType())) {
       AllBases.emplace_back(BaseClassDecl);
       if (!BaseClassDecl->field_empty() &&
-          utils::type_traits::isTriviallyDefaultConstructible(
-              Base.getType(), Context))
+          utils::type_traits::isTriviallyDefaultConstructible(Base.getType(),
+                                                              Context))
         BasesToInit.insert(BaseClassDecl);
     }
   }

clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init-cxx98.cpp

@@ -103,3 +103,14 @@ public:
 
   int X;
 };
+
+class PositiveIndirectMember {
+  struct {
+    int *A;
+  };
+
+  PositiveIndirectMember() : A() {}
+  PositiveIndirectMember(int) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: A
+  // CHECK-FIXES: PositiveIndirectMember(int) : A() {}
+};

clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp

@@ -357,3 +357,13 @@ class PositiveSelfInitialization : NegativeAggregateType
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these bases: NegativeAggregateType
   // CHECK-FIXES: PositiveSelfInitialization() : NegativeAggregateType(), PositiveSelfInitialization() {}
 };
+
+class PositiveIndirectMember {
+  struct {
+    int *A;
+    // CHECK-FIXES: int *A{};
+  };
+
+  PositiveIndirectMember() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: A
+};