Skip to content

[ntuple] Remove type name from public field constructors #19875

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+108 −111
Open

[ntuple] Remove type name from public field constructors #19875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
020309e
[ntuple] remove type name from atomic field constructor
jblomer Sep 11, 2025
92cbd1a
[ntuple] remove type name from public nullable field constructors
jblomer Sep 11, 2025
76818e9
[ntuple] remove unnecessary access specifier
jblomer Sep 12, 2025
6047b7e
[ntuple] remove type name from set field constructor
jblomer Sep 12, 2025
4d80ea9
[ntuple] remove type name from map field constructor
jblomer Sep 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 32 additions & 15 deletions tree/ntuple/inc/ROOT/RField/RFieldProxiedCollection.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,13 +150,10 @@ protected:
ROOT::Internal::RColumnIndex fNWritten;

/// Constructor used when the value type of the collection is not known in advance, i.e. in the case of custom
/// collections.
/// collections. Note that this constructor requires manual initialization of the item field
/// (Attach() and setting fItemSize)
RProxiedCollectionField(std::string_view fieldName, TClass *classp);
/// Constructor used when the value type of the collection is known in advance, e.g. in RSetField.
RProxiedCollectionField(std::string_view fieldName, std::string_view typeName,
std::unique_ptr<RFieldBase> itemField);

protected:
std::unique_ptr<RFieldBase> CloneImpl(std::string_view newName) const final;
const RColumnRepresentations &GetColumnRepresentations() const final;
void GenerateColumns() final;
Expand Down Expand Up @@ -284,7 +281,13 @@ public:
/// The generic field for a `std::map<KeyType, ValueType>` and `std::unordered_map<KeyType, ValueType>`
class RMapField : public RProxiedCollectionField {
public:
RMapField(std::string_view fieldName, std::string_view typeName, std::unique_ptr<RFieldBase> itemField);
enum class EMapType {
kMap,
kUnorderedMap,
kMultiMap,
kUnorderedMultiMap
};
RMapField(std::string_view fieldName, EMapType mapType, std::unique_ptr<RFieldBase> itemField);
RMapField(RMapField &&other) = default;
RMapField &operator=(RMapField &&other) = default;
~RMapField() override = default;
Expand All @@ -299,7 +302,7 @@ public:
}

explicit RField(std::string_view name)
: RMapField(name, TypeName(), std::make_unique<RField<std::pair<KeyT, ValueT>>>("_0"))
: RMapField(name, EMapType::kMap, std::make_unique<RField<std::pair<KeyT, ValueT>>>("_0"))
{
}
RField(RField &&other) = default;
Expand All @@ -316,7 +319,7 @@ public:
}

explicit RField(std::string_view name)
: RMapField(name, TypeName(), std::make_unique<RField<std::pair<KeyT, ValueT>>>("_0"))
: RMapField(name, EMapType::kUnorderedMap, std::make_unique<RField<std::pair<KeyT, ValueT>>>("_0"))
{
}
RField(RField &&other) = default;
Expand All @@ -333,7 +336,7 @@ public:
}

explicit RField(std::string_view name)
: RMapField(name, TypeName(), std::make_unique<RField<std::pair<KeyT, ValueT>>>("_0"))
: RMapField(name, EMapType::kMultiMap, std::make_unique<RField<std::pair<KeyT, ValueT>>>("_0"))
{
}
RField(RField &&other) = default;
Expand All @@ -350,7 +353,7 @@ public:
}

explicit RField(std::string_view name)
: RMapField(name, TypeName(), std::make_unique<RField<std::pair<KeyT, ValueT>>>("_0"))
: RMapField(name, EMapType::kUnorderedMultiMap, std::make_unique<RField<std::pair<KeyT, ValueT>>>("_0"))
{
}
RField(RField &&other) = default;
Expand All @@ -365,7 +368,13 @@ public:
/// The generic field for a `std::set<Type>` and `std::unordered_set<Type>`
class RSetField : public RProxiedCollectionField {
public:
RSetField(std::string_view fieldName, std::string_view typeName, std::unique_ptr<RFieldBase> itemField);
enum class ESetType {
kSet,
kUnorderedSet,
kMultiSet,
kUnorderedMultiSet
};
RSetField(std::string_view fieldName, ESetType setType, std::unique_ptr<RFieldBase> itemField);
RSetField(RSetField &&other) = default;
RSetField &operator=(RSetField &&other) = default;
~RSetField() override = default;
Expand All @@ -376,7 +385,7 @@ class RField<std::set<ItemT>> final : public RSetField {
public:
static std::string TypeName() { return "std::set<" + RField<ItemT>::TypeName() + ">"; }

explicit RField(std::string_view name) : RSetField(name, TypeName(), std::make_unique<RField<ItemT>>("_0")) {}
explicit RField(std::string_view name) : RSetField(name, ESetType::kSet, std::make_unique<RField<ItemT>>("_0")) {}
RField(RField &&other) = default;
RField &operator=(RField &&other) = default;
~RField() final = default;
Expand All @@ -387,7 +396,10 @@ class RField<std::unordered_set<ItemT>> final : public RSetField {
public:
static std::string TypeName() { return "std::unordered_set<" + RField<ItemT>::TypeName() + ">"; }

explicit RField(std::string_view name) : RSetField(name, TypeName(), std::make_unique<RField<ItemT>>("_0")) {}
explicit RField(std::string_view name)
: RSetField(name, ESetType::kUnorderedSet, std::make_unique<RField<ItemT>>("_0"))
{
}
RField(RField &&other) = default;
RField &operator=(RField &&other) = default;
~RField() final = default;
Expand All @@ -398,7 +410,9 @@ class RField<std::multiset<ItemT>> final : public RSetField {
public:
static std::string TypeName() { return "std::multiset<" + RField<ItemT>::TypeName() + ">"; }

explicit RField(std::string_view name) : RSetField(name, TypeName(), std::make_unique<RField<ItemT>>("_0")) {}
explicit RField(std::string_view name) : RSetField(name, ESetType::kMultiSet, std::make_unique<RField<ItemT>>("_0"))
{
}
RField(RField &&other) = default;
RField &operator=(RField &&other) = default;
~RField() final = default;
Expand All @@ -409,7 +423,10 @@ class RField<std::unordered_multiset<ItemT>> final : public RSetField {
public:
static std::string TypeName() { return "std::unordered_multiset<" + RField<ItemT>::TypeName() + ">"; }

explicit RField(std::string_view name) : RSetField(name, TypeName(), std::make_unique<RField<ItemT>>("_0")) {}
explicit RField(std::string_view name)
: RSetField(name, ESetType::kUnorderedMultiSet, std::make_unique<RField<ItemT>>("_0"))
{
}
RField(RField &&other) = default;
RField &operator=(RField &&other) = default;
~RField() final = default;
Expand Down
14 changes: 7 additions & 7 deletions tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ protected:
void ReconcileOnDiskField(const RNTupleDescriptor &desc) final;

public:
RAtomicField(std::string_view fieldName, std::string_view typeName, std::unique_ptr<RFieldBase> itemField);
RAtomicField(std::string_view fieldName, std::unique_ptr<RFieldBase> itemField);
RAtomicField(RAtomicField &&other) = default;
RAtomicField &operator=(RAtomicField &&other) = default;
~RAtomicField() override = default;
Expand All @@ -74,7 +74,7 @@ template <typename ItemT>
class RField<std::atomic<ItemT>> final : public RAtomicField {
public:
static std::string TypeName() { return "std::atomic<" + RField<ItemT>::TypeName() + ">"; }
explicit RField(std::string_view name) : RAtomicField(name, TypeName(), std::make_unique<RField<ItemT>>("_0")) {}
explicit RField(std::string_view name) : RAtomicField(name, std::make_unique<RField<ItemT>>("_0")) {}
RField(RField &&other) = default;
RField &operator=(RField &&other) = default;
~RField() final = default;
Expand Down Expand Up @@ -209,7 +209,7 @@ protected:
/// if it is null, returns `kInvalidNTupleIndex`
RNTupleLocalIndex GetItemIndex(ROOT::NTupleSize_t globalIndex);

RNullableField(std::string_view fieldName, std::string_view typeName, std::unique_ptr<RFieldBase> itemField);
RNullableField(std::string_view fieldName, const std::string &typePrefix, std::unique_ptr<RFieldBase> itemField);

public:
RNullableField(RNullableField &&other) = default;
Expand Down Expand Up @@ -249,7 +249,7 @@ protected:
void ReadGlobalImpl(ROOT::NTupleSize_t globalIndex, void *to) final;

public:
ROptionalField(std::string_view fieldName, std::string_view typeName, std::unique_ptr<RFieldBase> itemField);
ROptionalField(std::string_view fieldName, std::unique_ptr<RFieldBase> itemField);
ROptionalField(ROptionalField &&other) = default;
ROptionalField &operator=(ROptionalField &&other) = default;
~ROptionalField() override = default;
Expand All @@ -263,7 +263,7 @@ template <typename ItemT>
class RField<std::optional<ItemT>> final : public ROptionalField {
public:
static std::string TypeName() { return "std::optional<" + RField<ItemT>::TypeName() + ">"; }
explicit RField(std::string_view name) : ROptionalField(name, TypeName(), std::make_unique<RField<ItemT>>("_0")) {}
explicit RField(std::string_view name) : ROptionalField(name, std::make_unique<RField<ItemT>>("_0")) {}
RField(RField &&other) = default;
RField &operator=(RField &&other) = default;
~RField() final = default;
Expand Down Expand Up @@ -291,7 +291,7 @@ protected:
void ReadGlobalImpl(ROOT::NTupleSize_t globalIndex, void *to) final;

public:
RUniquePtrField(std::string_view fieldName, std::string_view typeName, std::unique_ptr<RFieldBase> itemField);
RUniquePtrField(std::string_view fieldName, std::unique_ptr<RFieldBase> itemField);
RUniquePtrField(RUniquePtrField &&other) = default;
RUniquePtrField &operator=(RUniquePtrField &&other) = default;
~RUniquePtrField() override = default;
Expand All @@ -305,7 +305,7 @@ template <typename ItemT>
class RField<std::unique_ptr<ItemT>> final : public RUniquePtrField {
public:
static std::string TypeName() { return "std::unique_ptr<" + RField<ItemT>::TypeName() + ">"; }
explicit RField(std::string_view name) : RUniquePtrField(name, TypeName(), std::make_unique<RField<ItemT>>("_0")) {}
explicit RField(std::string_view name) : RUniquePtrField(name, std::make_unique<RField<ItemT>>("_0")) {}
RField(RField &&other) = default;
RField &operator=(RField &&other) = default;
~RField() final = default;
Expand Down
27 changes: 13 additions & 14 deletions tree/ntuple/src/RField.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -761,9 +761,10 @@ void ROOT::RBitsetField::AcceptVisitor(ROOT::Detail::RFieldVisitor &visitor) con

//------------------------------------------------------------------------------

ROOT::RNullableField::RNullableField(std::string_view fieldName, std::string_view typeName,
ROOT::RNullableField::RNullableField(std::string_view fieldName, const std::string &typePrefix,
std::unique_ptr<RFieldBase> itemField)
: ROOT::RFieldBase(fieldName, typeName, ROOT::ENTupleStructure::kCollection, false /* isSimple */)
: ROOT::RFieldBase(fieldName, typePrefix + "<" + itemField->GetTypeName() + ">", ROOT::ENTupleStructure::kCollection,
Copy link
Member

@pcanal pcanal Sep 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As implied by:

This is dangerous as it potentially allows to write arbitrary type names to disk.

doesn't the 'prefix' then also need to be normalized? (At least here) wasn't the 'real' issue that the 2 types (typeName and the one derived from itemField->GetTypeName() being potentially different?

Copy link
Contributor Author

@jblomer jblomer Sep 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because the constructor that accepts a prefix is protected. I could have used the full type name instead of the type prefix approach. The reason I didn't is that the code calling this constructor would then both get the type name from the item field and move the item field to the base class. And doing both these things in a single call is illegal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't the previous constructor also protected?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the problem was not with RNullableField but with its descendants ROptionalField and RUniquePtrField that do have a public constructor.

false /* isSimple */)
{
Attach(std::move(itemField));
}
Expand Down Expand Up @@ -826,16 +827,15 @@ void ROOT::RNullableField::AcceptVisitor(ROOT::Detail::RFieldVisitor &visitor) c

//------------------------------------------------------------------------------

ROOT::RUniquePtrField::RUniquePtrField(std::string_view fieldName, std::string_view typeName,
std::unique_ptr<RFieldBase> itemField)
: RNullableField(fieldName, typeName, std::move(itemField)), fItemDeleter(GetDeleterOf(*fSubfields[0]))
ROOT::RUniquePtrField::RUniquePtrField(std::string_view fieldName, std::unique_ptr<RFieldBase> itemField)
: RNullableField(fieldName, "std::unique_ptr", std::move(itemField)), fItemDeleter(GetDeleterOf(*fSubfields[0]))
{
}

std::unique_ptr<ROOT::RFieldBase> ROOT::RUniquePtrField::CloneImpl(std::string_view newName) const
{
auto newItemField = fSubfields[0]->Clone(fSubfields[0]->GetFieldName());
return std::make_unique<RUniquePtrField>(newName, GetTypeName(), std::move(newItemField));
return std::make_unique<RUniquePtrField>(newName, std::move(newItemField));
}

std::size_t ROOT::RUniquePtrField::AppendImpl(const void *from)
Expand Down Expand Up @@ -905,9 +905,8 @@ std::vector<ROOT::RFieldBase::RValue> ROOT::RUniquePtrField::SplitValue(const RV

//------------------------------------------------------------------------------

ROOT::ROptionalField::ROptionalField(std::string_view fieldName, std::string_view typeName,
std::unique_ptr<RFieldBase> itemField)
: RNullableField(fieldName, typeName, std::move(itemField)), fItemDeleter(GetDeleterOf(*fSubfields[0]))
ROOT::ROptionalField::ROptionalField(std::string_view fieldName, std::unique_ptr<RFieldBase> itemField)
: RNullableField(fieldName, "std::optional", std::move(itemField)), fItemDeleter(GetDeleterOf(*fSubfields[0]))
{
if (fSubfields[0]->GetTraits() & kTraitTriviallyDestructible)
fTraits |= kTraitTriviallyDestructible;
Expand All @@ -926,7 +925,7 @@ const bool *ROOT::ROptionalField::GetEngagementPtr(const void *optionalPtr) cons
std::unique_ptr<ROOT::RFieldBase> ROOT::ROptionalField::CloneImpl(std::string_view newName) const
{
auto newItemField = fSubfields[0]->Clone(fSubfields[0]->GetFieldName());
return std::make_unique<ROptionalField>(newName, GetTypeName(), std::move(newItemField));
return std::make_unique<ROptionalField>(newName, std::move(newItemField));
}

std::size_t ROOT::ROptionalField::AppendImpl(const void *from)
Expand Down Expand Up @@ -1007,9 +1006,9 @@ size_t ROOT::ROptionalField::GetAlignment() const

//------------------------------------------------------------------------------

ROOT::RAtomicField::RAtomicField(std::string_view fieldName, std::string_view typeName,
std::unique_ptr<RFieldBase> itemField)
: RFieldBase(fieldName, typeName, ROOT::ENTupleStructure::kPlain, false /* isSimple */)
ROOT::RAtomicField::RAtomicField(std::string_view fieldName, std::unique_ptr<RFieldBase> itemField)
: RFieldBase(fieldName, "std::atomic<" + itemField->GetTypeName() + ">", ROOT::ENTupleStructure::kPlain,
false /* isSimple */)
{
if (itemField->GetTraits() & kTraitTriviallyConstructible)
fTraits |= kTraitTriviallyConstructible;
Expand All @@ -1021,7 +1020,7 @@ ROOT::RAtomicField::RAtomicField(std::string_view fieldName, std::string_view ty
std::unique_ptr<ROOT::RFieldBase> ROOT::RAtomicField::CloneImpl(std::string_view newName) const
{
auto newItemField = fSubfields[0]->Clone(fSubfields[0]->GetFieldName());
return std::make_unique<RAtomicField>(newName, GetTypeName(), std::move(newItemField));
return std::make_unique<RAtomicField>(newName, std::move(newItemField));
}

void ROOT::RAtomicField::ReconcileOnDiskField(const RNTupleDescriptor &desc)
Expand Down
Loading
Loading