Skip to content
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

[ntuple] Mark RField methods as final and remove duplicate implementations #16721

Merged
merged 10 commits into from
Oct 22, 2024
Merged

[ntuple] Mark RField methods as final and remove duplicate implementations #16721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
12a9580
[ntuple] Mark methods in final RField specializations
hahnjo Oct 19, 2024
1bbfff2
[ntuple] Mark some RField specializations final
hahnjo Oct 19, 2024
2ffaabc
[ntuple] Remove typed AppendImpl and ReadGlobalImpl for RVecs
hahnjo Oct 19, 2024
4d4dbee
[ntuple] Remove duplicate methods in RMapField
hahnjo Oct 19, 2024
ff1c924
[ntuple] Remove duplicate GetValueSize() and GetAlignment()
hahnjo Oct 19, 2024
297b4fe
[ntuple] Remove duplicate ConstructValue() and GetDeleter()
hahnjo Oct 19, 2024
73b4414
[ntuple] Remove unused RPairDeleter and RTupleDeleter
hahnjo Oct 19, 2024
e29a05d
[ntuple] Remove RMapField::fItemClass
hahnjo Oct 19, 2024
50b9002
[ntuple] Remove duplicate CloneImpl()
hahnjo Oct 19, 2024
5e6d5a5
[ntuple] Mark AcceptVisitor as final
hahnjo Oct 19, 2024
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
33 changes: 11 additions & 22 deletions tree/ntuple/v7/inc/ROOT/RField.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ class RFieldVisitor;
/// Therefore, the zero field must not be connected to a page source or sink.
class RFieldZero final : public RFieldBase {
protected:
std::unique_ptr<RFieldBase> CloneImpl(std::string_view newName) const override;
std::unique_ptr<RFieldBase> CloneImpl(std::string_view newName) const final;
void ConstructValue(void *) const final {}

public:
Expand Down Expand Up @@ -134,7 +134,7 @@ private:
protected:
std::unique_ptr<RFieldBase> CloneImpl(std::string_view newName) const final;

void ConstructValue(void *where) const override;
void ConstructValue(void *where) const final;
std::unique_ptr<RDeleter> GetDeleter() const final { return std::make_unique<RClassDeleter>(fClass); }

std::size_t AppendImpl(const void *from) final;
Expand All @@ -149,11 +149,11 @@ public:
~RClassField() override = default;

std::vector<RValue> SplitValue(const RValue &value) const final;
size_t GetValueSize() const override;
size_t GetValueSize() const final;
size_t GetAlignment() const final { return fMaxAlignment; }
std::uint32_t GetTypeVersion() const final;
std::uint32_t GetTypeChecksum() const final;
void AcceptVisitor(Detail::RFieldVisitor &visitor) const override;
void AcceptVisitor(Detail::RFieldVisitor &visitor) const final;
};

/// The field for a class in unsplit mode, which is using ROOT standard streaming
Expand Down Expand Up @@ -200,7 +200,7 @@ public:
RUnsplitField(std::string_view fieldName, std::string_view className, std::string_view typeAlias = "");
RUnsplitField(RUnsplitField &&other) = default;
RUnsplitField &operator=(RUnsplitField &&other) = default;
~RUnsplitField() override = default;
~RUnsplitField() final = default;

size_t GetValueSize() const final;
size_t GetAlignment() const final;
Expand Down Expand Up @@ -239,17 +239,6 @@ public:
/// Classes with dictionaries that can be inspected by TClass
template <typename T, typename = void>
class RField final : public RClassField {
protected:
void ConstructValue(void *where) const final
{
if constexpr (std::is_default_constructible_v<T>) {
new (where) T();
} else {
// If there is no default constructor, try with the IO constructor
new (where) T(static_cast<TRootIOCtor *>(nullptr));
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the tests still work, I assume TClass::New() takes proper care of using the I/O constructor where applicable...?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, in fact now I learned that TClass::New() allows more customization from the user: https://root.cern.ch/doc/master/classTClass.html#a1e952a88f08eb6ec665a7ac862860689. I cross-checked that removing the IO constructor of class IOConstructor in tree/ntuple/v7/test/CustomStruct.hxx fails the ntuple-types test, so it seems to be working with this PR.

Copy link
Member

@pcanal pcanal Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, this is essentially the whole point of TClass::New: construct the object during I/O operations :)
(and usually it does not require any run-time compilation/interpretation as the dictionary provide an accelerator function wrapping the 'right' call to operator new + constructor.

}
}

public:
static std::string TypeName() { return ROOT::Internal::GetDemangledTypeName(typeid(T)); }
RField(std::string_view name) : RClassField(name, TypeName())
Expand All @@ -258,17 +247,17 @@ public:
}
RField(RField &&other) = default;
RField &operator=(RField &&other) = default;
~RField() override = default;
~RField() final = default;
};

template <typename T>
class RField<T, typename std::enable_if<std::is_enum_v<T>>::type> : public REnumField {
class RField<T, typename std::enable_if<std::is_enum_v<T>>::type> final : public REnumField {
public:
static std::string TypeName() { return ROOT::Internal::GetDemangledTypeName(typeid(T)); }
RField(std::string_view name) : REnumField(name, TypeName()) {}
RField(RField &&other) = default;
RField &operator=(RField &&other) = default;
~RField() override = default;
~RField() final = default;
};

/// An artificial field that transforms an RNTuple column that contains the offset of collections into
Expand Down Expand Up @@ -368,7 +357,7 @@ public:
explicit RField(std::string_view name) : RCardinalityField(name, TypeName()) {}
RField(RField &&other) = default;
RField &operator=(RField &&other) = default;
~RField() override = default;
~RField() final = default;

size_t GetValueSize() const final { return sizeof(RNTupleCardinality<SizeT>); }
size_t GetAlignment() const final { return alignof(RNTupleCardinality<SizeT>); }
Expand Down Expand Up @@ -431,7 +420,7 @@ private:
protected:
std::unique_ptr<RFieldBase> CloneImpl(std::string_view newName) const final;

void ConstructValue(void *where) const override;
void ConstructValue(void *where) const final;
std::unique_ptr<RDeleter> GetDeleter() const final { return std::make_unique<RTypedDeleter<TObject>>(); }

std::size_t AppendImpl(const void *from) final;
Expand All @@ -445,7 +434,7 @@ public:
RField(std::string_view fieldName);
RField(RField &&other) = default;
RField &operator=(RField &&other) = default;
~RField() override = default;
~RField() final = default;

std::vector<RValue> SplitValue(const RValue &value) const final;
size_t GetValueSize() const final;
Expand Down
6 changes: 3 additions & 3 deletions tree/ntuple/v7/inc/ROOT/RField/RFieldFundamental.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public:
explicit RField(std::string_view name) : RSimpleField(name, TypeName()) {}
RField(RField &&other) = default;
RField &operator=(RField &&other) = default;
~RField() override = default;
~RField() final = default;

void AcceptVisitor(Detail::RFieldVisitor &visitor) const final;
};
Expand All @@ -96,7 +96,7 @@ public:
explicit RField(std::string_view name) : RSimpleField(name, TypeName()) {}
RField(RField &&other) = default;
RField &operator=(RField &&other) = default;
~RField() override = default;
~RField() final = default;

void AcceptVisitor(Detail::RFieldVisitor &visitor) const final;
};
Expand Down Expand Up @@ -328,7 +328,7 @@ public:
RField(std::string_view name) : RIntegralField<MappedType>(name) {}
RField(RField &&other) = default;
RField &operator=(RField &&other) = default;
~RField() override = default;
~RField() final = default;

T *Map(NTupleSize_t globalIndex) { return reinterpret_cast<T *>(this->BaseType::Map(globalIndex)); }
T *Map(RClusterIndex clusterIndex) { return reinterpret_cast<T *>(this->BaseType::Map(clusterIndex)); }
Expand Down
Loading
Loading