[ntuple] Fixes and cleanups for RRecordField and subclasses

tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx

@@ -46,7 +46,7 @@ private:
       std::vector<std::size_t> fOffsets;
 
    public:
-      RRecordDeleter(std::vector<std::unique_ptr<RDeleter>> &itemDeleters, const std::vector<std::size_t> &offsets)
+      RRecordDeleter(std::vector<std::unique_ptr<RDeleter>> itemDeleters, const std::vector<std::size_t> &offsets)
          : fItemDeleters(std::move(itemDeleters)), fOffsets(offsets)
       {
       }
@@ -71,29 +71,29 @@ protected:
    void ReadGlobalImpl(NTupleSize_t globalIndex, void *to) final;
    void ReadInClusterImpl(RClusterIndex clusterIndex, void *to) final;
 
-   RRecordField(std::string_view fieldName, std::vector<std::unique_ptr<RFieldBase>> &&itemFields,
-                const std::vector<std::size_t> &offsets, std::string_view typeName = "");
+   RRecordField(std::string_view fieldName, std::string_view typeName);
+
+   void AttachItemFields(std::vector<std::unique_ptr<RFieldBase>> itemFields);
 
    template <std::size_t N>
-   RRecordField(std::string_view fieldName, std::array<std::unique_ptr<RFieldBase>, N> &&itemFields,
-                const std::array<std::size_t, N> &offsets, std::string_view typeName = "")
-      : ROOT::Experimental::RFieldBase(fieldName, typeName, ENTupleStructure::kRecord, false /* isSimple */)
+   void AttachItemFields(std::array<std::unique_ptr<RFieldBase>, N> itemFields)
    {
       fTraits |= kTraitTrivialType;
       for (unsigned i = 0; i < N; ++i) {
-         fOffsets.push_back(offsets[i]);
          fMaxAlignment = std::max(fMaxAlignment, itemFields[i]->GetAlignment());
          fSize += GetItemPadding(fSize, itemFields[i]->GetAlignment()) + itemFields[i]->GetValueSize();
          fTraits &= itemFields[i]->GetTraits();
          Attach(std::move(itemFields[i]));
       }
+      // Trailing padding: although this is implementation-dependent, most add enough padding to comply with the
+      // requirements of the type with strictest alignment
+      fSize += GetItemPadding(fSize, fMaxAlignment);
    }
 
 public:
    /// Construct a RRecordField based on a vector of child fields. The ownership of the child fields is transferred
    /// to the RRecordField instance.
-   RRecordField(std::string_view fieldName, std::vector<std::unique_ptr<RFieldBase>> &&itemFields);
-   RRecordField(std::string_view fieldName, std::vector<std::unique_ptr<RFieldBase>> &itemFields);
+   RRecordField(std::string_view fieldName, std::vector<std::unique_ptr<RFieldBase>> itemFields);
    RRecordField(RRecordField &&other) = default;
    RRecordField &operator=(RRecordField &&other) = default;
    ~RRecordField() override = default;
@@ -116,11 +116,11 @@ private:
    static std::string GetTypeList(const std::array<std::unique_ptr<RFieldBase>, 2> &itemFields);
 
 protected:
-   RPairField(std::string_view fieldName, std::array<std::unique_ptr<RFieldBase>, 2> &&itemFields,
+   RPairField(std::string_view fieldName, std::array<std::unique_ptr<RFieldBase>, 2> itemFields,
               const std::array<std::size_t, 2> &offsets);
 
 public:
-   RPairField(std::string_view fieldName, std::array<std::unique_ptr<RFieldBase>, 2> &itemFields);
+   RPairField(std::string_view fieldName, std::array<std::unique_ptr<RFieldBase>, 2> itemFields);
    RPairField(RPairField &&other) = default;
    RPairField &operator=(RPairField &&other) = default;
    ~RPairField() override = default;
@@ -131,10 +131,9 @@ class RField<std::pair<T1, T2>> final : public RPairField {
    using ContainerT = typename std::pair<T1, T2>;
 
 private:
-   template <typename Ty1, typename Ty2>
    static std::array<std::unique_ptr<RFieldBase>, 2> BuildItemFields()
    {
-      return {std::make_unique<RField<Ty1>>("_0"), std::make_unique<RField<Ty2>>("_1")};
+      return {std::make_unique<RField<T1>>("_0"), std::make_unique<RField<T2>>("_1")};
    }
 
    static std::array<std::size_t, 2> BuildItemOffsets()
@@ -147,13 +146,11 @@ private:
 
 public:
    static std::string TypeName() { return "std::pair<" + RField<T1>::TypeName() + "," + RField<T2>::TypeName() + ">"; }
-   explicit RField(std::string_view name, std::array<std::unique_ptr<RFieldBase>, 2> &&itemFields)
-      : RPairField(name, std::move(itemFields), BuildItemOffsets())
+   explicit RField(std::string_view name) : RPairField(name, BuildItemFields(), BuildItemOffsets())
    {
-      fMaxAlignment = std::max(alignof(T1), alignof(T2));
-      fSize = sizeof(ContainerT);
+      R__ASSERT(fMaxAlignment >= std::max(alignof(T1), alignof(T2)));
+      R__ASSERT(fSize >= sizeof(ContainerT));
    }
-   explicit RField(std::string_view name) : RField(name, BuildItemFields<T1, T2>()) {}
    RField(RField &&other) = default;
    RField &operator=(RField &&other) = default;
    ~RField() final = default;
@@ -169,11 +166,11 @@ private:
    static std::string GetTypeList(const std::vector<std::unique_ptr<RFieldBase>> &itemFields);
 
 protected:
-   RTupleField(std::string_view fieldName, std::vector<std::unique_ptr<RFieldBase>> &&itemFields,
+   RTupleField(std::string_view fieldName, std::vector<std::unique_ptr<RFieldBase>> itemFields,
                const std::vector<std::size_t> &offsets);
 
 public:
-   RTupleField(std::string_view fieldName, std::vector<std::unique_ptr<RFieldBase>> &itemFields);
+   RTupleField(std::string_view fieldName, std::vector<std::unique_ptr<RFieldBase>> itemFields);
    RTupleField(RTupleField &&other) = default;
    RTupleField &operator=(RTupleField &&other) = default;
    ~RTupleField() override = default;
@@ -200,11 +197,10 @@ private:
       if constexpr (sizeof...(TailTs) > 0)
          _BuildItemFields<TailTs...>(itemFields, index + 1);
    }
-   template <typename... Ts>
    static std::vector<std::unique_ptr<RFieldBase>> BuildItemFields()
    {
       std::vector<std::unique_ptr<RFieldBase>> result;
-      _BuildItemFields<Ts...>(result);
+      _BuildItemFields<ItemTs...>(result);
       return result;
    }
 
@@ -217,23 +213,20 @@ private:
       if constexpr (sizeof...(TailTs) > 0)
          _BuildItemOffsets<Index + 1, TailTs...>(offsets, tuple);
    }
-   template <typename... Ts>
    static std::vector<std::size_t> BuildItemOffsets()
    {
       std::vector<std::size_t> result;
-      _BuildItemOffsets<0, Ts...>(result, ContainerT());
+      _BuildItemOffsets<0, ItemTs...>(result, ContainerT());
       return result;
    }
 
 public:
    static std::string TypeName() { return "std::tuple<" + BuildItemTypes<ItemTs...>() + ">"; }
-   explicit RField(std::string_view name, std::vector<std::unique_ptr<RFieldBase>> &&itemFields)
-      : RTupleField(name, std::move(itemFields), BuildItemOffsets<ItemTs...>())
+   explicit RField(std::string_view name) : RTupleField(name, BuildItemFields(), BuildItemOffsets())
    {
-      fMaxAlignment = std::max({alignof(ItemTs)...});
-      fSize = sizeof(ContainerT);
+      R__ASSERT(fMaxAlignment >= std::max({alignof(ItemTs)...}));
+      R__ASSERT(fSize >= sizeof(ContainerT));
    }
-   explicit RField(std::string_view name) : RField(name, BuildItemFields<ItemTs...>()) {}
    RField(RField &&other) = default;
    RField &operator=(RField &&other) = default;
    ~RField() final = default;

tree/ntuple/v7/src/RField.cxx

@@ -729,15 +729,15 @@ ROOT::Experimental::RFieldBase::Create(const std::string &fieldName, const std::
          }
          std::array<std::unique_ptr<RFieldBase>, 2> items{Create("_0", innerTypes[0]).Unwrap(),
                                                           Create("_1", innerTypes[1]).Unwrap()};
-         result = std::make_unique<RPairField>(fieldName, items);
+         result = std::make_unique<RPairField>(fieldName, std::move(items));
       } else if (canonicalType.substr(0, 11) == "std::tuple<") {
          auto innerTypes = TokenizeTypeList(canonicalType.substr(11, canonicalType.length() - 12));
          std::vector<std::unique_ptr<RFieldBase>> items;
          items.reserve(innerTypes.size());
          for (unsigned int i = 0; i < innerTypes.size(); ++i) {
             items.emplace_back(Create("_" + std::to_string(i), innerTypes[i]).Unwrap());
          }
-         result = std::make_unique<RTupleField>(fieldName, items);
+         result = std::make_unique<RTupleField>(fieldName, std::move(items));
       } else if (canonicalType.substr(0, 12) == "std::bitset<") {
          auto size = std::stoull(canonicalType.substr(12, canonicalType.length() - 13));
          result = std::make_unique<RBitsetField>(fieldName, size);
@@ -2507,11 +2507,13 @@ ROOT::Experimental::RRecordField::RRecordField(std::string_view name, const RRec
    fTraits = source.fTraits;
 }
 
-ROOT::Experimental::RRecordField::RRecordField(std::string_view fieldName,
-                                               std::vector<std::unique_ptr<RFieldBase>> &&itemFields,
-                                               const std::vector<std::size_t> &offsets, std::string_view typeName)
-   : ROOT::Experimental::RFieldBase(fieldName, typeName, ENTupleStructure::kRecord, false /* isSimple */),
-     fOffsets(offsets)
+ROOT::Experimental::RRecordField::RRecordField(std::string_view fieldName, std::string_view typeName)
+   : ROOT::Experimental::RFieldBase(fieldName, typeName, ENTupleStructure::kRecord, false /* isSimple */)
+{
+}
+
+void ROOT::Experimental::RRecordField::RRecordField::AttachItemFields(
+   std::vector<std::unique_ptr<RFieldBase>> itemFields)
 {
    fTraits |= kTraitTrivialType;
    for (auto &item : itemFields) {
@@ -2520,10 +2522,13 @@ ROOT::Experimental::RRecordField::RRecordField(std::string_view fieldName,
       fTraits &= item->GetTraits();
       Attach(std::move(item));
    }
+   // Trailing padding: although this is implementation-dependent, most add enough padding to comply with the
+   // requirements of the type with strictest alignment
+   fSize += GetItemPadding(fSize, fMaxAlignment);
 }
 
 ROOT::Experimental::RRecordField::RRecordField(std::string_view fieldName,
-                                               std::vector<std::unique_ptr<RFieldBase>> &&itemFields)
+                                               std::vector<std::unique_ptr<RFieldBase>> itemFields)
    : ROOT::Experimental::RFieldBase(fieldName, "", ENTupleStructure::kRecord, false /* isSimple */)
 {
    fTraits |= kTraitTrivialType;
@@ -2541,12 +2546,6 @@ ROOT::Experimental::RRecordField::RRecordField(std::string_view fieldName,
    fSize += GetItemPadding(fSize, fMaxAlignment);
 }
 
-ROOT::Experimental::RRecordField::RRecordField(std::string_view fieldName,
-                                               std::vector<std::unique_ptr<RFieldBase>> &itemFields)
-   : ROOT::Experimental::RRecordField(fieldName, std::move(itemFields))
-{
-}
-
 std::size_t ROOT::Experimental::RRecordField::GetItemPadding(std::size_t baseOffset, std::size_t itemAlignment) const
 {
    if (itemAlignment > 1) {
@@ -2608,7 +2607,7 @@ std::unique_ptr<ROOT::Experimental::RFieldBase::RDeleter> ROOT::Experimental::RR
    for (const auto &f : fSubFields) {
       itemDeleters.emplace_back(GetDeleterOf(*f));
    }
-   return std::make_unique<RRecordDeleter>(itemDeleters, fOffsets);
+   return std::make_unique<RRecordDeleter>(std::move(itemDeleters), fOffsets);
 }
 
 std::vector<ROOT::Experimental::RFieldBase::RValue>
@@ -3868,18 +3867,21 @@ ROOT::Experimental::RPairField::RPairField::GetTypeList(const std::array<std::un
 }
 
 ROOT::Experimental::RPairField::RPairField(std::string_view fieldName,
-                                           std::array<std::unique_ptr<RFieldBase>, 2> &&itemFields,
+                                           std::array<std::unique_ptr<RFieldBase>, 2> itemFields,
                                            const std::array<std::size_t, 2> &offsets)
-   : ROOT::Experimental::RRecordField(fieldName, std::move(itemFields), offsets,
-                                      "std::pair<" + GetTypeList(itemFields) + ">")
+   : ROOT::Experimental::RRecordField(fieldName, "std::pair<" + GetTypeList(itemFields) + ">")
 {
+   AttachItemFields(std::move(itemFields));
+   fOffsets.push_back(offsets[0]);
+   fOffsets.push_back(offsets[1]);
 }
 
 ROOT::Experimental::RPairField::RPairField(std::string_view fieldName,
-                                           std::array<std::unique_ptr<RFieldBase>, 2> &itemFields)
-   : ROOT::Experimental::RRecordField(fieldName, std::move(itemFields), {},
-                                      "std::pair<" + GetTypeList(itemFields) + ">")
+                                           std::array<std::unique_ptr<RFieldBase>, 2> itemFields)
+   : ROOT::Experimental::RRecordField(fieldName, "std::pair<" + GetTypeList(itemFields) + ">")
 {
+   AttachItemFields(std::move(itemFields));
+
    // ISO C++ does not guarantee any specific layout for `std::pair`; query TClass for the member offsets
    auto *c = TClass::GetClass(GetTypeName().c_str());
    if (!c)
@@ -3889,12 +3891,12 @@ ROOT::Experimental::RPairField::RPairField(std::string_view fieldName,
    auto firstElem = c->GetRealData("first");
    if (!firstElem)
       throw RException(R__FAIL("first: no such member"));
-   fOffsets[0] = firstElem->GetThisOffset();
+   fOffsets.push_back(firstElem->GetThisOffset());
 
    auto secondElem = c->GetRealData("second");
    if (!secondElem)
       throw RException(R__FAIL("second: no such member"));
-   fOffsets[1] = secondElem->GetThisOffset();
+   fOffsets.push_back(secondElem->GetThisOffset());
 }
 
 //------------------------------------------------------------------------------
@@ -3913,18 +3915,20 @@ ROOT::Experimental::RTupleField::RTupleField::GetTypeList(const std::vector<std:
 }
 
 ROOT::Experimental::RTupleField::RTupleField(std::string_view fieldName,
-                                             std::vector<std::unique_ptr<RFieldBase>> &&itemFields,
+                                             std::vector<std::unique_ptr<RFieldBase>> itemFields,
                                              const std::vector<std::size_t> &offsets)
-   : ROOT::Experimental::RRecordField(fieldName, std::move(itemFields), offsets,
-                                      "std::tuple<" + GetTypeList(itemFields) + ">")
+   : ROOT::Experimental::RRecordField(fieldName, "std::tuple<" + GetTypeList(itemFields) + ">")
 {
+   AttachItemFields(std::move(itemFields));
+   fOffsets = offsets;
 }
 
 ROOT::Experimental::RTupleField::RTupleField(std::string_view fieldName,
-                                             std::vector<std::unique_ptr<RFieldBase>> &itemFields)
-   : ROOT::Experimental::RRecordField(fieldName, std::move(itemFields), {},
-                                      "std::tuple<" + GetTypeList(itemFields) + ">")
+                                             std::vector<std::unique_ptr<RFieldBase>> itemFields)
+   : ROOT::Experimental::RRecordField(fieldName, "std::tuple<" + GetTypeList(itemFields) + ">")
 {
+   AttachItemFields(std::move(itemFields));
+
    auto *c = TClass::GetClass(GetTypeName().c_str());
    if (!c)
       throw RException(R__FAIL("cannot get type information for " + GetTypeName()));

tree/ntuple/v7/src/RNTupleDescriptor.cxx

@@ -97,7 +97,7 @@ ROOT::Experimental::RFieldDescriptor::CreateField(const RNTupleDescriptor &ntplD
                return field;
             memberFields.emplace_back(std::move(field));
          }
-         auto recordField = std::make_unique<RRecordField>(GetFieldName(), memberFields);
+         auto recordField = std::make_unique<RRecordField>(GetFieldName(), std::move(memberFields));
          recordField->SetOnDiskId(fFieldId);
          return recordField;
       }

tree/ntuple/v7/test/ntuple_descriptor.cxx

@@ -594,7 +594,8 @@ TEST(RNTupleDescriptor, BuildStreamerInfos)
    streamerInfoMap = fnBuildStreamerInfosOf(*RFieldBase::Create("f", "std::unordered_map<int, float>").Unwrap());
    EXPECT_TRUE(streamerInfoMap.empty());
 
-   streamerInfoMap = fnBuildStreamerInfosOf(ROOT::Experimental::RRecordField("f", {}));
+   std::vector<std::unique_ptr<RFieldBase>> itemFields;
+   streamerInfoMap = fnBuildStreamerInfosOf(ROOT::Experimental::RRecordField("f", std::move(itemFields)));
    EXPECT_TRUE(streamerInfoMap.empty());
 
    streamerInfoMap = fnBuildStreamerInfosOf(*RFieldBase::Create("f", "CustomStruct").Unwrap());

tree/ntuple/v7/test/ntuple_show.cxx

@@ -349,7 +349,7 @@ TEST(RNTupleShow, Collections)
       std::vector<std::unique_ptr<RFieldBase>> leafFields;
       leafFields.emplace_back(std::make_unique<RField<short>>("myShort"));
       leafFields.emplace_back(std::make_unique<RField<float>>("myFloat"));
-      auto recordField = std::make_unique<RRecordField>("_0", leafFields);
+      auto recordField = std::make_unique<RRecordField>("_0", std::move(leafFields));
       EXPECT_EQ(offsetof(MyStruct, myShort), recordField->GetOffsets()[0]);
       EXPECT_EQ(offsetof(MyStruct, myFloat), recordField->GetOffsets()[1]);
 

tree/ntuple/v7/test/ntuple_types.cxx

@@ -138,7 +138,7 @@ TEST(RNTuple, CreateField)
    std::vector<std::unique_ptr<RFieldBase>> itemFields;
    itemFields.push_back(std::make_unique<RField<std::uint32_t>>("u32"));
    itemFields.push_back(std::make_unique<RField<std::uint8_t>>("u8"));
-   ROOT::Experimental::RRecordField record("test", itemFields);
+   ROOT::Experimental::RRecordField record("test", std::move(itemFields));
    EXPECT_EQ(alignof(std::uint32_t), record.GetAlignment());
    // Check that trailing padding is added after `u8` to comply with the alignment requirements of uint32_t
    EXPECT_EQ(sizeof(std::uint32_t) + alignof(std::uint32_t), record.GetValueSize());
@@ -307,6 +307,14 @@ TEST(RNTuple, StdTuple)
          "tupleTupleField");
    EXPECT_STREQ("std::tuple<std::tuple<std::int64_t,float,char,float>,std::vector<std::tuple<char,char,char>>>",
                 tupleTupleField.GetTypeName().c_str());
+   using TupleMapSetType = std::tuple<std::map<char, int64_t>, std::set<int64_t>>;
+   auto tupleMapSetField = RField<TupleMapSetType>("tupleMapSetField");
+   EXPECT_LE(sizeof(TupleMapSetType), tupleMapSetField.GetValueSize());
+   EXPECT_LE(alignof(TupleMapSetType), tupleMapSetField.GetAlignment());
+   using TupleUnorderedMapSetType = std::tuple<std::unordered_map<char, int64_t>, std::unordered_set<int64_t>>;
+   auto tupleUnorderedMapSetField = RField<TupleUnorderedMapSetType>("tupleUnorderedMapSetField");
+   EXPECT_LE(sizeof(TupleUnorderedMapSetType), tupleUnorderedMapSetField.GetValueSize());
+   EXPECT_LE(alignof(TupleUnorderedMapSetType), tupleUnorderedMapSetField.GetAlignment());
 
    FileRaii fileGuard("test_ntuple_rfield_stdtuple.root");
    {

tree/ntupleutil/v7/src/RNTupleImporter.cxx

@@ -317,7 +317,7 @@ ROOT::Experimental::RResult<void> ROOT::Experimental::RNTupleImporter::PrepareSc
       auto &c = p.second;
 
       c.fFieldName = "_collection" + std::to_string(iLeafCountCollection);
-      auto recordField = std::make_unique<RRecordField>("_0", c.fLeafFields);
+      auto recordField = std::make_unique<RRecordField>("_0", std::move(c.fLeafFields));
       c.fRecordField = recordField.get();
       auto collectionField = RVectorField::CreateUntyped(c.fFieldName, std::move(recordField));
       fModel->AddField(std::move(collectionField));