DataStructure root node

[mmarineBlueQuartz]

• DataStructure now derives from BaseGroup to directly serve as its own root node.
• Updated BaseGroup to prevent adding a DataStructure as a child node.
• Changed BaseGroup methods to vert allow DataStructure to override specific methods.

[JDuffeyBQ]

Maybe we can go over some of the details/goals/implications after our usual meeting?

[imikejackson]

Review of PR #1428: "DataStructure root node"

Overview

This PR makes DataStructure inherit from BaseGroup (→ DataObject) so it serves as its own root node, eliminating the separate DataMap m_RootGroup member. The core idea is sound, but there are two critical bugs and some design concerns.

---

Critical Issues

1. Compile Error: getTopLevelData() still references removed m_RootGroup

The PR removes DataMap m_RootGroup; from the member variables, but getTopLevelData() was never updated — it still references m_RootGroup directly:

DataStructure.cpp (current lines 511-521):

std::vector<DataObject*> DataStructure::getTopLevelData() const
{
  std::vector<DataObject*> topLevel(m_RootGroup.getSize(), nullptr);  // COMPILE ERROR
  usize index = 0;
  for(auto& iter : m_RootGroup)  // COMPILE ERROR
  {
    ...
  }
}

Both references need to be changed to getDataMap().

2. Undefined Behavior: generateId() called before m_NextId is initialized

The new default constructor:

DataStructure::DataStructure()
: BaseGroup(*this, k_TypeName)  // ← calls generateId() on *this
, m_IsValid(true)
{}

C++ initializes base classes before member variables. BaseGroup(*this, k_TypeName) chains into DataObject(DataStructure& ds, std::string name) which calls dataStructure.generateId(). At this point, m_NextId has not been initialized yet (it's a DataStructure member, initialized after the BaseGroup base). generateId() reads and increments uninitialized m_NextId — this is undefined behavior.

Fix: The DataStructure needs to use the DataObject constructor overload that takes an explicit ID (e.g., pass 0 as a reserved ID), bypassing generateId(). Or restructure so m_NextId initialization happens before the base class construction (e.g., move it to a base class or use a different initialization strategy).

---

Design Concerns

3. DataStructure is now a DataObject — identity side effects

Since DataStructure now IS-A DataObject, it has:

• An getId() (with garbage value due to issue #2)
• A getName() ("DataStructure")
• A parent list (m_ParentList)
• Methods like getDataPaths(), isGroup(), getDataObjectType(), etc.

Any code that iterates over DataObjects, does dynamic_cast<BaseGroup*>, or traverses parent/child relationships needs to account for the fact that the DataStructure itself is now a DataObject. While the DataStructure isn't tracked in m_DataObjects (so getData(id) won't find it), this is a non-obvious invariant that could easily be violated in the future.

4. Destruction order is correct but fragile

The destruction sequence works because:

1. ~DataStructure() sets m_IsValid = false and nulls all DataObject pointers
2. ~BaseGroup() cleans up the DataMap (children already have null DataStructure pointers)
3. ~DataObject() checks m_DataStructure == this (the new guard) and returns early

The m_DataStructure == this check in ~DataObject() is essential and correct — without it, the DataStructure would call dataDeleted() on itself during destruction. However, this is a subtle invariant that deserves a comment explaining why it's needed.

---

What's Good

• The canInsert() guard preventing DataStructure from being added as a BaseGroup child is correct
• The noexcept addition to BaseGroup(BaseGroup&&) is appropriate
• Making clear() virtual so DataStructure can override is clean
• The deepCopy() returning nullptr and shallowCopy() returning new DataStructure(*this) are reasonable
• Most m_RootGroup → getDataMap() conversions are correct

---

Minor

• PR description typo: "Changed BaseGroup methods to vert allow" → "to allow"

---

Verdict

The two critical issues (#1 compile error, #2 undefined behavior) must be fixed before this can merge. Issue #3 (DataStructure identity) warrants broader discussion about downstream impact across the codebase.