diff --git a/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx b/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx index a55a2347ff21b..02a9484d0f9f3 100644 --- a/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx +++ b/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx @@ -46,7 +46,7 @@ private: std::vector fOffsets; public: - RRecordDeleter(std::vector> &itemDeleters, const std::vector &offsets) + RRecordDeleter(std::vector> itemDeleters, const std::vector &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> &&itemFields, - const std::vector &offsets, std::string_view typeName = ""); + RRecordField(std::string_view fieldName, std::string_view typeName); + + void AttachItemFields(std::vector> itemFields); template - RRecordField(std::string_view fieldName, std::array, N> &&itemFields, - const std::array &offsets, std::string_view typeName = "") - : ROOT::Experimental::RFieldBase(fieldName, typeName, ENTupleStructure::kRecord, false /* isSimple */) + void AttachItemFields(std::array, 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> &&itemFields); - RRecordField(std::string_view fieldName, std::vector> &itemFields); + RRecordField(std::string_view fieldName, std::vector> 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, 2> &itemFields); protected: - RPairField(std::string_view fieldName, std::array, 2> &&itemFields, + RPairField(std::string_view fieldName, std::array, 2> itemFields, const std::array &offsets); public: - RPairField(std::string_view fieldName, std::array, 2> &itemFields); + RPairField(std::string_view fieldName, std::array, 2> itemFields); RPairField(RPairField &&other) = default; RPairField &operator=(RPairField &&other) = default; ~RPairField() override = default; @@ -131,10 +131,9 @@ class RField> final : public RPairField { using ContainerT = typename std::pair; private: - template static std::array, 2> BuildItemFields() { - return {std::make_unique>("_0"), std::make_unique>("_1")}; + return {std::make_unique>("_0"), std::make_unique>("_1")}; } static std::array BuildItemOffsets() @@ -147,13 +146,11 @@ private: public: static std::string TypeName() { return "std::pair<" + RField::TypeName() + "," + RField::TypeName() + ">"; } - explicit RField(std::string_view name, std::array, 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()) {} RField(RField &&other) = default; RField &operator=(RField &&other) = default; ~RField() final = default; @@ -169,11 +166,11 @@ private: static std::string GetTypeList(const std::vector> &itemFields); protected: - RTupleField(std::string_view fieldName, std::vector> &&itemFields, + RTupleField(std::string_view fieldName, std::vector> itemFields, const std::vector &offsets); public: - RTupleField(std::string_view fieldName, std::vector> &itemFields); + RTupleField(std::string_view fieldName, std::vector> itemFields); RTupleField(RTupleField &&other) = default; RTupleField &operator=(RTupleField &&other) = default; ~RTupleField() override = default; @@ -200,11 +197,10 @@ private: if constexpr (sizeof...(TailTs) > 0) _BuildItemFields(itemFields, index + 1); } - template static std::vector> BuildItemFields() { std::vector> result; - _BuildItemFields(result); + _BuildItemFields(result); return result; } @@ -217,23 +213,20 @@ private: if constexpr (sizeof...(TailTs) > 0) _BuildItemOffsets(offsets, tuple); } - template static std::vector BuildItemOffsets() { std::vector result; - _BuildItemOffsets<0, Ts...>(result, ContainerT()); + _BuildItemOffsets<0, ItemTs...>(result, ContainerT()); return result; } public: static std::string TypeName() { return "std::tuple<" + BuildItemTypes() + ">"; } - explicit RField(std::string_view name, std::vector> &&itemFields) - : RTupleField(name, std::move(itemFields), BuildItemOffsets()) + 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()) {} RField(RField &&other) = default; RField &operator=(RField &&other) = default; ~RField() final = default; diff --git a/tree/ntuple/v7/src/RField.cxx b/tree/ntuple/v7/src/RField.cxx index 5843d11a1dc45..b4dbd0622cd5e 100644 --- a/tree/ntuple/v7/src/RField.cxx +++ b/tree/ntuple/v7/src/RField.cxx @@ -729,7 +729,7 @@ ROOT::Experimental::RFieldBase::Create(const std::string &fieldName, const std:: } std::array, 2> items{Create("_0", innerTypes[0]).Unwrap(), Create("_1", innerTypes[1]).Unwrap()}; - result = std::make_unique(fieldName, items); + result = std::make_unique(fieldName, std::move(items)); } else if (canonicalType.substr(0, 11) == "std::tuple<") { auto innerTypes = TokenizeTypeList(canonicalType.substr(11, canonicalType.length() - 12)); std::vector> items; @@ -737,7 +737,7 @@ ROOT::Experimental::RFieldBase::Create(const std::string &fieldName, const std:: for (unsigned int i = 0; i < innerTypes.size(); ++i) { items.emplace_back(Create("_" + std::to_string(i), innerTypes[i]).Unwrap()); } - result = std::make_unique(fieldName, items); + result = std::make_unique(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(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> &&itemFields, - const std::vector &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> 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> &&itemFields) + std::vector> 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> &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::RR for (const auto &f : fSubFields) { itemDeleters.emplace_back(GetDeleterOf(*f)); } - return std::make_unique(itemDeleters, fOffsets); + return std::make_unique(std::move(itemDeleters), fOffsets); } std::vector @@ -3868,18 +3867,21 @@ ROOT::Experimental::RPairField::RPairField::GetTypeList(const std::array, 2> &&itemFields, + std::array, 2> itemFields, const std::array &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, 2> &itemFields) - : ROOT::Experimental::RRecordField(fieldName, std::move(itemFields), {}, - "std::pair<" + GetTypeList(itemFields) + ">") + std::array, 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> &&itemFields, + std::vector> itemFields, const std::vector &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> &itemFields) - : ROOT::Experimental::RRecordField(fieldName, std::move(itemFields), {}, - "std::tuple<" + GetTypeList(itemFields) + ">") + std::vector> 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())); diff --git a/tree/ntuple/v7/src/RNTupleDescriptor.cxx b/tree/ntuple/v7/src/RNTupleDescriptor.cxx index 78c9f5326ba63..b4ea684f6366d 100644 --- a/tree/ntuple/v7/src/RNTupleDescriptor.cxx +++ b/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(GetFieldName(), memberFields); + auto recordField = std::make_unique(GetFieldName(), std::move(memberFields)); recordField->SetOnDiskId(fFieldId); return recordField; } diff --git a/tree/ntuple/v7/test/ntuple_descriptor.cxx b/tree/ntuple/v7/test/ntuple_descriptor.cxx index 15fad274ce9f1..15348abf8da3d 100644 --- a/tree/ntuple/v7/test/ntuple_descriptor.cxx +++ b/tree/ntuple/v7/test/ntuple_descriptor.cxx @@ -594,7 +594,8 @@ TEST(RNTupleDescriptor, BuildStreamerInfos) streamerInfoMap = fnBuildStreamerInfosOf(*RFieldBase::Create("f", "std::unordered_map").Unwrap()); EXPECT_TRUE(streamerInfoMap.empty()); - streamerInfoMap = fnBuildStreamerInfosOf(ROOT::Experimental::RRecordField("f", {})); + std::vector> itemFields; + streamerInfoMap = fnBuildStreamerInfosOf(ROOT::Experimental::RRecordField("f", std::move(itemFields))); EXPECT_TRUE(streamerInfoMap.empty()); streamerInfoMap = fnBuildStreamerInfosOf(*RFieldBase::Create("f", "CustomStruct").Unwrap()); diff --git a/tree/ntuple/v7/test/ntuple_show.cxx b/tree/ntuple/v7/test/ntuple_show.cxx index edea1fa4ce7ab..f12a44f8c2981 100644 --- a/tree/ntuple/v7/test/ntuple_show.cxx +++ b/tree/ntuple/v7/test/ntuple_show.cxx @@ -349,7 +349,7 @@ TEST(RNTupleShow, Collections) std::vector> leafFields; leafFields.emplace_back(std::make_unique>("myShort")); leafFields.emplace_back(std::make_unique>("myFloat")); - auto recordField = std::make_unique("_0", leafFields); + auto recordField = std::make_unique("_0", std::move(leafFields)); EXPECT_EQ(offsetof(MyStruct, myShort), recordField->GetOffsets()[0]); EXPECT_EQ(offsetof(MyStruct, myFloat), recordField->GetOffsets()[1]); diff --git a/tree/ntuple/v7/test/ntuple_types.cxx b/tree/ntuple/v7/test/ntuple_types.cxx index 14b0e8d741c6d..79d48ba359c35 100644 --- a/tree/ntuple/v7/test/ntuple_types.cxx +++ b/tree/ntuple/v7/test/ntuple_types.cxx @@ -138,7 +138,7 @@ TEST(RNTuple, CreateField) std::vector> itemFields; itemFields.push_back(std::make_unique>("u32")); itemFields.push_back(std::make_unique>("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::vector>>", tupleTupleField.GetTypeName().c_str()); + using TupleMapSetType = std::tuple, std::set>; + auto tupleMapSetField = RField("tupleMapSetField"); + EXPECT_LE(sizeof(TupleMapSetType), tupleMapSetField.GetValueSize()); + EXPECT_LE(alignof(TupleMapSetType), tupleMapSetField.GetAlignment()); + using TupleUnorderedMapSetType = std::tuple, std::unordered_set>; + auto tupleUnorderedMapSetField = RField("tupleUnorderedMapSetField"); + EXPECT_LE(sizeof(TupleUnorderedMapSetType), tupleUnorderedMapSetField.GetValueSize()); + EXPECT_LE(alignof(TupleUnorderedMapSetType), tupleUnorderedMapSetField.GetAlignment()); FileRaii fileGuard("test_ntuple_rfield_stdtuple.root"); { diff --git a/tree/ntupleutil/v7/src/RNTupleImporter.cxx b/tree/ntupleutil/v7/src/RNTupleImporter.cxx index 9712c81009734..2d93b38dcfe69 100644 --- a/tree/ntupleutil/v7/src/RNTupleImporter.cxx +++ b/tree/ntupleutil/v7/src/RNTupleImporter.cxx @@ -317,7 +317,7 @@ ROOT::Experimental::RResult ROOT::Experimental::RNTupleImporter::PrepareSc auto &c = p.second; c.fFieldName = "_collection" + std::to_string(iLeafCountCollection); - auto recordField = std::make_unique("_0", c.fLeafFields); + auto recordField = std::make_unique("_0", std::move(c.fLeafFields)); c.fRecordField = recordField.get(); auto collectionField = RVectorField::CreateUntyped(c.fFieldName, std::move(recordField)); fModel->AddField(std::move(collectionField));