[clang][dataflow] Introduce a helper class for handling record initializer lists.

[martinboehme]

This is currently only used in one place, but I'm working on a patch that will
use this from a second place. And I think this already improves the readability
of the one place this is used so far.

[llvmbot]

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: None (martinboehme)

Changes

This is currently only used in one place, but I'm working on a patch that will
use this from a second place. And I think this already improves the readability
of the one place this is used so far.

---

Full diff: https://github.com/llvm/llvm-project/pull/86675.diff

3 Files Affected:

• (modified) clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h (+29)
• (modified) clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp (+36)
• (modified) clang/lib/Analysis/FlowSensitive/Transfer.cpp (+16-45)

diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index 2330697299fdd7..c30bccd06674a4 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -744,6 +744,35 @@ RecordStorageLocation *getBaseObjectLocation(const MemberExpr &ME,
 std::vector<const FieldDecl *>
 getFieldsForInitListExpr(const InitListExpr *InitList);
 
+/// Helper class for initialization of a record with an `InitListExpr`.
+/// `InitListExpr::inits()` contains the initializers for both the base classes
+/// and the fields of the record; this helper class separates these out into two
+/// different lists. In addition, it deals with special cases associated with
+/// unions.
+class RecordInitListHelper {
+public:
+  // `InitList` must have record type.
+  RecordInitListHelper(const InitListExpr *InitList);
+
+  // Base classes with their associated initializer expressions.
+  ArrayRef<std::pair<const CXXBaseSpecifier *, Expr *>> base_inits() const {
+    return BaseInits;
+  }
+
+  // Fields with their associated initializer expressions.
+  ArrayRef<std::pair<const FieldDecl *, Expr *>> field_inits() const {
+    return FieldInits;
+  }
+
+private:
+  SmallVector<std::pair<const CXXBaseSpecifier *, Expr *>> BaseInits;
+  SmallVector<std::pair<const FieldDecl *, Expr *>> FieldInits;
+
+  // We potentially synthesize an `ImplicitValueInitExpr` for unions. It's a
+  // member variable because we store a pointer to it in `FieldInits`.
+  std::optional<ImplicitValueInitExpr> ImplicitValueInitForUnion;
+};
+
 /// Associates a new `RecordValue` with `Loc` and returns the new value.
 RecordValue &refreshRecordValue(RecordStorageLocation &Loc, Environment &Env);
 
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index cc1ebd511191a9..fcce117961ae38 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -1169,6 +1169,42 @@ getFieldsForInitListExpr(const InitListExpr *InitList) {
   return Fields;
 }
 
+RecordInitListHelper::RecordInitListHelper(const InitListExpr *InitList) {
+  auto *RD = InitList->getType()->getAsCXXRecordDecl();
+  assert(RD != nullptr);
+
+  std::vector<const FieldDecl *> Fields = getFieldsForInitListExpr(InitList);
+  ArrayRef<Expr *> Inits = InitList->inits();
+
+  // Unions initialized with an empty initializer list need special treatment.
+  // For structs/classes initialized with an empty initializer list, Clang
+  // puts `ImplicitValueInitExpr`s in `InitListExpr::inits()`, but for unions,
+  // it doesn't do this -- so we create an `ImplicitValueInitExpr` ourselves.
+  SmallVector<Expr *> InitsForUnion;
+  if (InitList->getType()->isUnionType() && Inits.empty()) {
+    assert(Fields.size() == 1);
+    ImplicitValueInitForUnion.emplace(Fields.front()->getType());
+    InitsForUnion.push_back(&*ImplicitValueInitForUnion);
+    Inits = InitsForUnion;
+  }
+
+  size_t InitIdx = 0;
+
+  assert(Fields.size() + RD->getNumBases() == Inits.size());
+  for (const CXXBaseSpecifier &Base : RD->bases()) {
+    assert(InitIdx < Inits.size());
+    Expr *Init = Inits[InitIdx++];
+    BaseInits.emplace_back(&Base, Init);
+  }
+
+  assert(Fields.size() == Inits.size() - InitIdx);
+  for (const FieldDecl *Field : Fields) {
+    assert(InitIdx < Inits.size());
+    Expr *Init = Inits[InitIdx++];
+    FieldInits.emplace_back(Field, Init);
+  }
+}
+
 RecordValue &refreshRecordValue(RecordStorageLocation &Loc, Environment &Env) {
   auto &NewVal = Env.create<RecordValue>(Loc);
   Env.setValue(Loc, NewVal);
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 960e9688ffb725..0a2e8368d541dd 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -689,51 +689,22 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
     }
 
     llvm::DenseMap<const ValueDecl *, StorageLocation *> FieldLocs;
-
-    // This only contains the direct fields for the given type.
-    std::vector<const FieldDecl *> FieldsForInit = getFieldsForInitListExpr(S);
-
-    // `S->inits()` contains all the initializer expressions, including the
-    // ones for direct base classes.
-    ArrayRef<Expr *> Inits = S->inits();
-    size_t InitIdx = 0;
-
-    // Unions initialized with an empty initializer list need special treatment.
-    // For structs/classes initialized with an empty initializer list, Clang
-    // puts `ImplicitValueInitExpr`s in `InitListExpr::inits()`, but for unions,
-    // it doesn't do this -- so we create an `ImplicitValueInitExpr` ourselves.
-    std::optional<ImplicitValueInitExpr> ImplicitValueInitForUnion;
-    SmallVector<Expr *> InitsForUnion;
-    if (S->getType()->isUnionType() && Inits.empty()) {
-      assert(FieldsForInit.size() == 1);
-      ImplicitValueInitForUnion.emplace(FieldsForInit.front()->getType());
-      InitsForUnion.push_back(&*ImplicitValueInitForUnion);
-      Inits = InitsForUnion;
-    }
-
-    // Initialize base classes.
-    if (auto* R = S->getType()->getAsCXXRecordDecl()) {
-      assert(FieldsForInit.size() + R->getNumBases() == Inits.size());
-      for ([[maybe_unused]] const CXXBaseSpecifier &Base : R->bases()) {
-        assert(InitIdx < Inits.size());
-        auto Init = Inits[InitIdx++];
-        assert(Base.getType().getCanonicalType() ==
-               Init->getType().getCanonicalType());
-        auto *BaseVal = Env.get<RecordValue>(*Init);
-        if (!BaseVal)
-          BaseVal = cast<RecordValue>(Env.createValue(Init->getType()));
-        // Take ownership of the fields of the `RecordValue` for the base class
-        // and incorporate them into the "flattened" set of fields for the
-        // derived class.
-        auto Children = BaseVal->getLoc().children();
-        FieldLocs.insert(Children.begin(), Children.end());
-      }
-    }
-
-    assert(FieldsForInit.size() == Inits.size() - InitIdx);
-    for (auto Field : FieldsForInit) {
-      assert(InitIdx < Inits.size());
-      auto Init = Inits[InitIdx++];
+    RecordInitListHelper InitListHelper(S);
+
+    for (auto [Base, Init] : InitListHelper.base_inits()) {
+      assert(Base->getType().getCanonicalType() ==
+             Init->getType().getCanonicalType());
+      auto *BaseVal = Env.get<RecordValue>(*Init);
+      if (!BaseVal)
+        BaseVal = cast<RecordValue>(Env.createValue(Init->getType()));
+      // Take ownership of the fields of the `RecordValue` for the base class
+      // and incorporate them into the "flattened" set of fields for the
+      // derived class.
+      auto Children = BaseVal->getLoc().children();
+      FieldLocs.insert(Children.begin(), Children.end());
+    }
+
+    for (auto [Field, Init] : InitListHelper.field_inits()) {
       assert(
           // The types are same, or
           Field->getType().getCanonicalType().getUnqualifiedType() ==

[ymand]

Why an object vs something like:

struct RecordInits {
  SmallVector<std::pair<const CXXBaseSpecifier *, Expr *>> BaseInits;
  SmallVector<std::pair<const FieldDecl *, Expr *>> FieldInits;
};
RecordInits RecordInitListHelper(const InitListExpr *InitList);

Is it because of the optional storage needed for the union?

[martinboehme]

Is it because of the optional storage needed for the union?

Yes, exactly this -- otherwise, there would be no way for us to keep the ImplicitValueInitExpr "alive".