Skip to content

Commit

Permalink
[ntuple] Take container of unique_ptr's by value
Browse files Browse the repository at this point in the history 
Similar to taking a single std::unique_ptr by value to signal ownership
handover, we should take containers thereof by value. This avoids having
two overloads and requires the user to explicitly move the container of
item fields instead of modifying it when passed by reference.
  • Loading branch information
@hahnjo
hahnjo committed Oct 28, 2024
1 parent 8a4278a commit 9a9d233
Show file tree
Hide file tree
Showing 6 changed files with 21 additions and 28 deletions.
17 changes: 8 additions & 9 deletions tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
}
Expand All @@ -73,10 +73,10 @@ protected:

RRecordField(std::string_view fieldName, std::string_view typeName);

void AttachItemFields(std::vector<std::unique_ptr<RFieldBase>> &&itemFields);
void AttachItemFields(std::vector<std::unique_ptr<RFieldBase>> itemFields);

template <std::size_t N>
void AttachItemFields(std::array<std::unique_ptr<RFieldBase>, N> &&itemFields)
void AttachItemFields(std::array<std::unique_ptr<RFieldBase>, N> itemFields)
{
fTraits |= kTraitTrivialType;
for (unsigned i = 0; i < N; ++i) {
Expand All @@ -90,8 +90,7 @@ protected:
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;
Expand All @@ -114,11 +113,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;
Expand Down Expand Up @@ -165,11 +164,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;
Expand Down
24 changes: 9 additions & 15 deletions tree/ntuple/v7/src/RField.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -2513,7 +2513,7 @@ ROOT::Experimental::RRecordField::RRecordField(std::string_view fieldName, std::
}

void ROOT::Experimental::RRecordField::RRecordField::AttachItemFields(
std::vector<std::unique_ptr<RFieldBase>> &&itemFields)
std::vector<std::unique_ptr<RFieldBase>> itemFields)
{
fTraits |= kTraitTrivialType;
for (auto &item : itemFields) {
Expand All @@ -2525,7 +2525,7 @@ void ROOT::Experimental::RRecordField::RRecordField::AttachItemFields(
}

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;
Expand All @@ -2543,12 +2543,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) {
Expand Down Expand Up @@ -2610,7 +2604,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>
Expand Down Expand Up @@ -3870,7 +3864,7 @@ 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::pair<" + GetTypeList(itemFields) + ">")
{
Expand All @@ -3880,7 +3874,7 @@ ROOT::Experimental::RPairField::RPairField(std::string_view fieldName,
}

ROOT::Experimental::RPairField::RPairField(std::string_view fieldName,
std::array<std::unique_ptr<RFieldBase>, 2> &itemFields)
std::array<std::unique_ptr<RFieldBase>, 2> itemFields)
: ROOT::Experimental::RRecordField(fieldName, "std::pair<" + GetTypeList(itemFields) + ">")
{
AttachItemFields(std::move(itemFields));
Expand Down Expand Up @@ -3918,7 +3912,7 @@ 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::tuple<" + GetTypeList(itemFields) + ">")
{
Expand All @@ -3927,7 +3921,7 @@ ROOT::Experimental::RTupleField::RTupleField(std::string_view fieldName,
}

ROOT::Experimental::RTupleField::RTupleField(std::string_view fieldName,
std::vector<std::unique_ptr<RFieldBase>> &itemFields)
std::vector<std::unique_ptr<RFieldBase>> itemFields)
: ROOT::Experimental::RRecordField(fieldName, "std::tuple<" + GetTypeList(itemFields) + ">")
{
AttachItemFields(std::move(itemFields));
Expand Down
2 changes: 1 addition & 1 deletion tree/ntuple/v7/src/RNTupleDescriptor.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion tree/ntuple/v7/test/ntuple_show.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]);

Expand Down
2 changes: 1 addition & 1 deletion tree/ntuple/v7/test/ntuple_types.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
2 changes: 1 addition & 1 deletion tree/ntupleutil/v7/src/RNTupleImporter.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down

0 comments on commit 9a9d233

Please sign in to comment.