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

[michael_miller]

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.

[alexfh]

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp

31 ↗

(On Diff #56341)

Is getCanonicalType() important here? Did you try using QualType::getAsCXXRecordDecl()?

[michael_miller]

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp

31 ↗

(On Diff #56341)

Probably not but I didn't try it. I just moved the previous function up to the top so I didn't have to forward declare it.

One thing I'm simply unsure of is whether it's possible to get a RecordDecl that's not a CXXRecordDecl in C++ code. It seems like the answer is no but that might be another reason to keep it as is if I'm wrong about that...

[hokein]

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp

31 ↗

(On Diff #56341)

One thing I'm simply unsure of is whether it's possible to get a RecordDecl that's not a CXXRecordDecl in C++ code.

From the documents, RecordDecl and CXXRecordDecl present struct/union/class, except CXXRecordDecl provides more methods for classes. So I think you can try QualType::getAsCXXRecordDecl() here.

[michael_miller]

Switched to using getAsCXXRecordDecl.

[hokein]

LGTM.

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

 using llvm::SmallPtrSetImpl;
 
 namespace clang {
 namespace tidy {
 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;
   }
 }
 
 void removeFieldsInitializedInBody(
     const Stmt &Stmt, ASTContext &Context,
     SmallPtrSetImpl<const FieldDecl *> &FieldDecls) {
   auto Matches =
       match(findAll(binaryOperator(
computeInsertions(const CXXConstructorDecl::init_const_range &Inits,
   for (const CXXCtorInitializer *Init : Inits) {
     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<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.
       for (; Decl != std::end(OrderedDecls) && *Decl != InitDecl; ++Decl) {
         if (const T *D = dyn_cast<T>(*Decl)) {
           if (DeclsToInit.count(D) > 0)
             Insertions.back().Initializers.emplace_back(getName(D));
         }
void getInitializationsInOrder(const CXXRecordDecl *ClassDecl,
                                SmallVectorImpl<const NamedDecl *> &Decls) {
   Decls.clear();
   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);
     }
   }
-  Decls.append(ClassDecl->fields().begin(), ClassDecl->fields().end());
+  forEachField(ClassDecl, ClassDecl->fields(), false,
+               [&](const FieldDecl *F) { Decls.push_back(F); });
 }
 
 template <typename T>
 void fixInitializerList(const ASTContext &Context, DiagnosticBuilder &Diag,
                         const CXXConstructorDecl *Ctor,
                         const SmallPtrSetImpl<const T *> &DeclsToInit) {
   // Do not propose fixes in macros since we cannot place them correctly.
   if (Ctor->getLocStart().isMacroID())
     return;
 
   SmallVector<const NamedDecl *, 16> 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 <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,
                                                ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       IgnoreArrays(Options.get("IgnoreArrays", false)) {}
 
 void ProTypeMemberInitCheck::registerMatchers(MatchFinder *Finder) {

 void ProTypeMemberInitCheck::checkMissingMemberInitializer(
     ASTContext &Context, const CXXConstructorDecl *Ctor) {
   const CXXRecordDecl *ClassDecl = Ctor->getParent();
   bool IsUnion = ClassDecl->isUnion();
 
   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;
 
   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());
+      FieldsToInit.erase(Init->getAnyMember());
     }
   }
   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,
void ProTypeMemberInitCheck::checkMissingBaseClassInitializer(
 
   // 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()) {
     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);
     }
   }
 
   if (BasesToInit.empty())
     return;
 
   // Remove any bases that were explicitly written in the initializer list.

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

 class PositiveTemplateBase : T {
 public:
   PositiveTemplateBase() {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: X
   // CHECK-FIXES: PositiveTemplateBase() : X() {}
 
   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/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp

 // initializer list.
 template <typename T>
 class PositiveSelfInitialization : NegativeAggregateType
 {
   PositiveSelfInitialization() : PositiveSelfInitialization() {}
   // 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
+};