From e142a0a1a84464b9e58315f351bc1899831bbf1b Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Sat, 22 Nov 2025 00:36:44 +0100 Subject: [PATCH 1/5] [ntuple] add Internal::NeedsMetaNameAsAlias() --- tree/ntuple/inc/ROOT/RFieldUtils.hxx | 8 ++++++ tree/ntuple/src/RFieldUtils.cxx | 36 ++++++++++++++++++++++++++- tree/ntuple/test/ntuple_type_name.cxx | 16 ++++++++++++ 3 files changed, 59 insertions(+), 1 deletion(-) diff --git a/tree/ntuple/inc/ROOT/RFieldUtils.hxx b/tree/ntuple/inc/ROOT/RFieldUtils.hxx index 5724777da3573..4fdff2f3f83d4 100644 --- a/tree/ntuple/inc/ROOT/RFieldUtils.hxx +++ b/tree/ntuple/inc/ROOT/RFieldUtils.hxx @@ -35,6 +35,14 @@ std::string GetRenormalizedTypeName(const std::string &metaNormalizedName); /// ensure that e.g. fundamental types are normalized to the type used by RNTuple (e.g. int -> std::int32_t). std::string GetRenormalizedTypeName(const std::type_info &ti); +/// Checks if the meta normalized name is different from the RNTuple normalized name in a way that would cause +/// the RNTuple normalized name to request different streamer info. This can happen, e.g., if the type name has +/// Long64_t as a template parameter. In this case, RNTuple should use the meta normalized name as a type alias +/// to ensure correct reconstruction of objects from disk. +/// If the function returns true, renormalizedAlias contains the RNTuple normalized name that should be used as +/// type alias. +bool NeedsMetaNameAsAlias(const std::string &metaNormalizedName, std::string &renormalizedAlias); + /// Applies all RNTuple type normalization rules except typedef resolution. std::string GetNormalizedUnresolvedTypeName(const std::string &origName); diff --git a/tree/ntuple/src/RFieldUtils.cxx b/tree/ntuple/src/RFieldUtils.cxx index f2f1a287da452..d168c56a7a396 100644 --- a/tree/ntuple/src/RFieldUtils.cxx +++ b/tree/ntuple/src/RFieldUtils.cxx @@ -62,7 +62,8 @@ const std::unordered_map typeTranslationMap{ {"ULong64_t", "unsigned long long"}, {"uint64_t", "std::uint64_t"}}; -// Natively supported types drop the default template arguments and the CV qualifiers in template arguments. +// Natively supported types drop the default template arguments and the CV qualifiers in template arguments; +// the also keep Long64_t as template argument untouched. bool IsUserClass(const std::string &typeName) { return typeName.rfind("std::", 0) != 0 && typeName.rfind("ROOT::VecOps::RVec<", 0) != 0; @@ -531,6 +532,39 @@ std::string ROOT::Internal::GetRenormalizedTypeName(const std::string &metaNorma return GetRenormalizedMetaTypeName(metaNormalizedName); } +bool ROOT::Internal::NeedsMetaNameAsAlias(const std::string &metaNormalizedName, std::string &renormalizedAlias) +{ + const auto canonicalTypePrefix = ROOT::Internal::GetCanonicalTypePrefix(metaNormalizedName); + if (canonicalTypePrefix.find('<') == std::string::npos) { + // If there are no templates, the function is done. + return false; + } + + bool result = false; + bool isTemplatedUserClass = IsUserClass(canonicalTypePrefix); + auto fnCheckLong64 = [&](const std::string &arg) -> std::string { + if ((arg == "Long64_t" || arg == "ULong64_t") && isTemplatedUserClass) { + result = true; + return arg; + } + + std::string renormalizedArgAlias; + if (NeedsMetaNameAsAlias(arg, renormalizedArgAlias)) { + result = true; + return renormalizedArgAlias; + } + + const auto renormalizedArg = GetRenormalizedMetaTypeName(arg); + isTemplatedUserClass = (renormalizedArg.find('<') == std::string::npos) && IsUserClass(renormalizedArg); + return renormalizedArg; + }; + + renormalizedAlias = canonicalTypePrefix; + NormalizeTemplateArguments(renormalizedAlias, 0 /* maxTemplateArgs */, fnCheckLong64); + + return result; +} + std::string ROOT::Internal::GetNormalizedUnresolvedTypeName(const std::string &origName) { const TClassEdit::EModType modType = static_cast( diff --git a/tree/ntuple/test/ntuple_type_name.cxx b/tree/ntuple/test/ntuple_type_name.cxx index 44c6bd66d563f..553d1fc815e2c 100644 --- a/tree/ntuple/test/ntuple_type_name.cxx +++ b/tree/ntuple/test/ntuple_type_name.cxx @@ -380,3 +380,19 @@ TEST(RNTuple, ContextDependentTypeNames) } } } + +TEST(RNTuple, NeedsMetaNameAsAlias) +{ + using ROOT::Internal::NeedsMetaNameAsAlias; + + std::string renormalizedAlias; + EXPECT_FALSE(NeedsMetaNameAsAlias("bool", renormalizedAlias)); + EXPECT_FALSE(NeedsMetaNameAsAlias("std::vector", renormalizedAlias)); + EXPECT_FALSE(NeedsMetaNameAsAlias("std::vector", renormalizedAlias)); + EXPECT_TRUE(NeedsMetaNameAsAlias("::MyClass", renormalizedAlias)); + EXPECT_EQ("MyClass", renormalizedAlias); + EXPECT_TRUE(NeedsMetaNameAsAlias("MyClass", renormalizedAlias)); + EXPECT_TRUE(NeedsMetaNameAsAlias("std::vector >", renormalizedAlias)); + EXPECT_EQ("std::vector>", renormalizedAlias); + EXPECT_FALSE(NeedsMetaNameAsAlias("MyClass>", renormalizedAlias)); +} From 051e95b3de23f7abaff1db253f098b454bab4c90 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Sat, 22 Nov 2025 00:37:22 +0100 Subject: [PATCH 2/5] [ntuple] fix type names with [U]Long64_t template args Custom classes with [U]Long64_t template arguments need to use their meta-normalized name as type alias. Otherwise, during reconstruction with the RNTuple normalized name, the streamer info for the `std::[u]int64_t` argument will be requested (typically `long` instead of `long long`). --- tree/ntuple/src/RFieldMeta.cxx | 4 +++ tree/ntuple/test/CustomStruct.hxx | 7 +++- tree/ntuple/test/CustomStructLinkDef.h | 2 +- tree/ntuple/test/rfield_class.cxx | 44 ++++++++++++++++---------- 4 files changed, 38 insertions(+), 19 deletions(-) diff --git a/tree/ntuple/src/RFieldMeta.cxx b/tree/ntuple/src/RFieldMeta.cxx index f524169362970..9b79c9d5265f6 100644 --- a/tree/ntuple/src/RFieldMeta.cxx +++ b/tree/ntuple/src/RFieldMeta.cxx @@ -158,6 +158,10 @@ ROOT::RClassField::RClassField(std::string_view fieldName, TClass *classp) if (!(fClass->ClassProperty() & kClassHasExplicitDtor)) fTraits |= kTraitTriviallyDestructible; + std::string renormalizedAlias; + if (Internal::NeedsMetaNameAsAlias(classp->GetName(), renormalizedAlias)) + fTypeAlias = renormalizedAlias; + int i = 0; const auto *bases = fClass->GetListOfBases(); assert(bases); diff --git a/tree/ntuple/test/CustomStruct.hxx b/tree/ntuple/test/CustomStruct.hxx index 135fdfae150ce..df1c5e6aab4a6 100644 --- a/tree/ntuple/test/CustomStruct.hxx +++ b/tree/ntuple/test/CustomStruct.hxx @@ -100,14 +100,19 @@ struct alignas(std::uint64_t) TestEBO : public EmptyStruct { template class EdmWrapper { public: + struct Inner { + T fX; + }; + bool fIsPresent = true; T fMember; }; class EdmContainer { public: + using EdmWrapperLong64_t = EdmWrapper; // Used to test that the streamer info for fWrapper will use long long - EdmWrapper fWrapper; + EdmWrapperLong64_t fWrapper; }; template diff --git a/tree/ntuple/test/CustomStructLinkDef.h b/tree/ntuple/test/CustomStructLinkDef.h index dc7d0f6e7ed65..5fe31dd100db3 100644 --- a/tree/ntuple/test/CustomStructLinkDef.h +++ b/tree/ntuple/test/CustomStructLinkDef.h @@ -33,7 +33,7 @@ #pragma link C++ class EdmWrapper +; #pragma link C++ class EdmHash < 1> + ; #pragma link C++ class EdmWrapper+; -#pragma link C++ class EdmContainer; +#pragma link C++ class EdmContainer+; #pragma link C++ class DataVector < int, double> + ; #pragma link C++ class DataVector < int, float> + ; diff --git a/tree/ntuple/test/rfield_class.cxx b/tree/ntuple/test/rfield_class.cxx index cdb13f62b273d..c810fdab85a6a 100644 --- a/tree/ntuple/test/rfield_class.cxx +++ b/tree/ntuple/test/rfield_class.cxx @@ -395,29 +395,32 @@ TEST(RNTuple, TClassMetaName) TEST(RNTuple, StreamerInfoRecords) { - // Every testee consists of the type stored on disk and the expected streamer info records - std::vector>> testees{ - {"float", {}}, - {"std::vector", {}}, - {"std::pair", {}}, - {"std::map", {}}, - {"CustomStruct", {"CustomStruct"}}, - {"std::vector", {"CustomStruct"}}, - {"std::map", {"CustomStruct"}}, - {"DerivedA", {"DerivedA", "CustomStruct"}}, - {"std::pair", {"DerivedA", "CustomStruct"}}, - {"EdmWrapper", {"EdmWrapper"}}, - {"TRotation", {"TRotation"}}}; + // Every testee consists of the type stored on disk, the expected streamer info records, and the expected type alias + std::vector, std::string>> testees{ + {"float", {}, ""}, + {"std::vector", {}, ""}, + {"std::pair", {}, ""}, + {"std::map", {}, ""}, + {"CustomStruct", {"CustomStruct"}, ""}, + {"std::vector", {"CustomStruct"}, ""}, + {"std::map", {"CustomStruct"}, ""}, + {"DerivedA", {"DerivedA", "CustomStruct"}, ""}, + {"std::pair", {"DerivedA", "CustomStruct"}, ""}, + {"EdmWrapper", {"EdmWrapper"}, "EdmWrapper"}, + {"EdmContainer", {"EdmContainer", "EdmWrapper"}, ""}, + {"EdmWrapper::Inner", {"EdmWrapper::Inner"}, "EdmWrapper::Inner"}, + {"EdmContainer::EdmWrapperLong64_t", {"EdmWrapper"}, "EdmContainer::EdmWrapperLong64_t"}, + {"TRotation", {"TRotation"}, ""}}; for (const auto &t : testees) { FileRaii fileGuard("test_ntuple_streamer_info_records.root"); { auto model = ROOT::RNTupleModel::Create(); - if (t.first == "TRotation") { - model->AddField(std::make_unique("f", t.first)); + if (std::get<0>(t) == "TRotation") { + model->AddField(std::make_unique("f", std::get<0>(t))); } else { - model->AddField(ROOT::RFieldBase::Create("f", t.first).Unwrap()); + model->AddField(ROOT::RFieldBase::Create("f", std::get<0>(t)).Unwrap()); } auto writer = ROOT::RNTupleWriter::Recreate(std::move(model), "ntpl", fileGuard.GetPath()); } @@ -425,7 +428,7 @@ TEST(RNTuple, StreamerInfoRecords) auto f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str())); ASSERT_TRUE(f && !f->IsZombie()); - std::unordered_set expectedInfos{t.second.begin(), t.second.end()}; + std::unordered_set expectedInfos{std::get<1>(t).begin(), std::get<1>(t).end()}; expectedInfos.insert("ROOT::RNTuple"); for (const auto info : TRangeDynCast(*f->GetStreamerInfoList())) { auto itr = expectedInfos.find(info->GetName()); @@ -436,5 +439,12 @@ TEST(RNTuple, StreamerInfoRecords) expectedInfos.erase(itr); } EXPECT_TRUE(expectedInfos.empty()); + + // Make sure we can reconstruct the fields + auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath()); + EXPECT_EQ(std::get<2>(t), reader->GetModel().GetConstField("f").GetTypeAlias()); + if (auto field = dynamic_cast(&reader->GetModel().GetConstField("f"))) { + EXPECT_EQ(std::get<1>(t)[0], field->GetClass()->GetName()); + } } } From e06d3764849f5bd25b8f5142512708b071693c1e Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Sun, 23 Nov 2025 23:33:24 +0100 Subject: [PATCH 3/5] [ntuple] fix collection proxy with [U]Long64_t template arg --- tree/ntuple/inc/ROOT/RField/RFieldProxiedCollection.hxx | 2 +- tree/ntuple/src/RFieldMeta.cxx | 8 ++++++-- tree/ntuple/test/CustomStructLinkDef.h | 1 + tree/ntuple/test/SimpleCollectionProxy.hxx | 7 +++++-- tree/ntuple/test/rfield_class.cxx | 9 +++++++++ 5 files changed, 22 insertions(+), 5 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RField/RFieldProxiedCollection.hxx b/tree/ntuple/inc/ROOT/RField/RFieldProxiedCollection.hxx index fe0d63b11364c..9ee3724b58b23 100644 --- a/tree/ntuple/inc/ROOT/RField/RFieldProxiedCollection.hxx +++ b/tree/ntuple/inc/ROOT/RField/RFieldProxiedCollection.hxx @@ -255,7 +255,7 @@ template class RField::value>::type> final : public RProxiedCollectionField { public: static std::string TypeName() { return ROOT::Internal::GetRenormalizedTypeName(typeid(T)); } - RField(std::string_view name) : RProxiedCollectionField(name, TypeName()) + RField(std::string_view name) : RProxiedCollectionField(name, Internal::GetDemangledTypeName(typeid(T))) { static_assert(std::is_class::value, "collection proxy unsupported for fundamental types"); } diff --git a/tree/ntuple/src/RFieldMeta.cxx b/tree/ntuple/src/RFieldMeta.cxx index 9b79c9d5265f6..1c2b80d4ca879 100644 --- a/tree/ntuple/src/RFieldMeta.cxx +++ b/tree/ntuple/src/RFieldMeta.cxx @@ -748,7 +748,7 @@ ROOT::RProxiedCollectionField::RProxiedCollectionField(std::string_view fieldNam fNWritten(0) { if (!classp->GetCollectionProxy()) - throw RException(R__FAIL(std::string(GetTypeName()) + " has no associated collection proxy")); + throw RException(R__FAIL(std::string(classp->GetName()) + " has no associated collection proxy")); if (classp->Property() & kIsDefinedInStd) { static const std::vector supportedStdTypes = { "std::set<", "std::unordered_set<", "std::multiset<", "std::unordered_multiset<", @@ -764,6 +764,10 @@ ROOT::RProxiedCollectionField::RProxiedCollectionField(std::string_view fieldNam throw RException(R__FAIL(std::string(GetTypeName()) + " is not supported")); } + std::string renormalizedAlias; + if (Internal::NeedsMetaNameAsAlias(classp->GetName(), renormalizedAlias)) + fTypeAlias = renormalizedAlias; + fProxy.reset(classp->GetCollectionProxy()->Generate()); fProperties = fProxy->GetProperties(); fCollectionType = fProxy->GetCollectionType(); @@ -801,7 +805,7 @@ ROOT::RProxiedCollectionField::RProxiedCollectionField(std::string_view fieldNam case EDataType::kFloat_t: itemField = std::make_unique>("_0"); break; case EDataType::kDouble_t: itemField = std::make_unique>("_0"); break; case EDataType::kBool_t: itemField = std::make_unique>("_0"); break; - default: throw RException(R__FAIL("unsupported value type")); + default: throw RException(R__FAIL("unsupported value type: " + std::to_string(fProxy->GetType()))); } } diff --git a/tree/ntuple/test/CustomStructLinkDef.h b/tree/ntuple/test/CustomStructLinkDef.h index 5fe31dd100db3..e4355ae91150b 100644 --- a/tree/ntuple/test/CustomStructLinkDef.h +++ b/tree/ntuple/test/CustomStructLinkDef.h @@ -67,6 +67,7 @@ #pragma link C++ class StructUsingCollectionProxy + ; #pragma link C++ class StructUsingCollectionProxy> + ; #pragma link C++ class StructUsingCollectionProxy + ; +#pragma link C++ class StructUsingCollectionProxy+; #pragma link C++ class TrivialTraitsBase + ; #pragma link C++ class TrivialTraits + ; diff --git a/tree/ntuple/test/SimpleCollectionProxy.hxx b/tree/ntuple/test/SimpleCollectionProxy.hxx index 462fdfc080e14..6d152f76c8482 100644 --- a/tree/ntuple/test/SimpleCollectionProxy.hxx +++ b/tree/ntuple/test/SimpleCollectionProxy.hxx @@ -72,6 +72,8 @@ public: return EDataType::kInt_t; if constexpr (std::is_same::value) return EDataType::kFloat_t; + if constexpr (std::is_same::value) + return EDataType::kLong64_t; return EDataType::kOther_t; } @@ -122,8 +124,9 @@ template <> struct IsCollectionProxy> : std::true_type { }; template <> -struct IsCollectionProxy> : std::true_type { -}; +struct IsCollectionProxy> : std::true_type {}; +template <> +struct IsCollectionProxy> : std::true_type {}; template <> struct IsCollectionProxy>> : std::true_type { diff --git a/tree/ntuple/test/rfield_class.cxx b/tree/ntuple/test/rfield_class.cxx index c810fdab85a6a..d6887f45eb17f 100644 --- a/tree/ntuple/test/rfield_class.cxx +++ b/tree/ntuple/test/rfield_class.cxx @@ -1,4 +1,5 @@ #include "ntuple_test.hxx" +#include "SimpleCollectionProxy.hxx" #include #include @@ -391,6 +392,14 @@ TEST(RNTuple, TClassMetaName) auto f4 = RFieldBase::Create("f", "EdmContainer").Unwrap(); EXPECT_STREQ("EdmWrapper", static_cast(f4->GetConstSubfields()[0])->GetClass()->GetName()); + + SimpleCollectionProxy> proxy; + auto cl = TClass::GetClass("StructUsingCollectionProxy"); + cl->CopyCollectionProxy(proxy); + + auto f5 = std::make_unique>>("f"); + EXPECT_TRUE(dynamic_cast(f5.get())); + EXPECT_EQ("StructUsingCollectionProxy", f5->GetTypeAlias()); } TEST(RNTuple, StreamerInfoRecords) From 4bf7473a7865315ab01adf7c24d550afa8076ec2 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Sun, 23 Nov 2025 23:39:19 +0100 Subject: [PATCH 4/5] [ntuple] strip unneeded type alias arg from streamer field constructor --- tree/ntuple/inc/ROOT/RField.hxx | 2 +- tree/ntuple/src/RFieldMeta.cxx | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RField.hxx b/tree/ntuple/inc/ROOT/RField.hxx index d4590349992f3..b8783121174f6 100644 --- a/tree/ntuple/inc/ROOT/RField.hxx +++ b/tree/ntuple/inc/ROOT/RField.hxx @@ -272,7 +272,7 @@ protected: void ReconcileOnDiskField(const RNTupleDescriptor &desc) final; public: - RStreamerField(std::string_view fieldName, std::string_view className, std::string_view typeAlias = ""); + RStreamerField(std::string_view fieldName, std::string_view className); RStreamerField(RStreamerField &&other) = default; RStreamerField &operator=(RStreamerField &&other) = default; ~RStreamerField() final = default; diff --git a/tree/ntuple/src/RFieldMeta.cxx b/tree/ntuple/src/RFieldMeta.cxx index 1c2b80d4ca879..67074b99b7abb 100644 --- a/tree/ntuple/src/RFieldMeta.cxx +++ b/tree/ntuple/src/RFieldMeta.cxx @@ -1010,10 +1010,9 @@ class TBufferRecStreamer : public TBufferFile { } // anonymous namespace -ROOT::RStreamerField::RStreamerField(std::string_view fieldName, std::string_view className, std::string_view typeAlias) +ROOT::RStreamerField::RStreamerField(std::string_view fieldName, std::string_view className) : RStreamerField(fieldName, EnsureValidClass(className)) { - fTypeAlias = typeAlias; } ROOT::RStreamerField::RStreamerField(std::string_view fieldName, TClass *classp) @@ -1034,7 +1033,7 @@ ROOT::RStreamerField::RStreamerField(std::string_view fieldName, TClass *classp) std::unique_ptr ROOT::RStreamerField::CloneImpl(std::string_view newName) const { - return std::unique_ptr(new RStreamerField(newName, GetTypeName(), GetTypeAlias())); + return std::unique_ptr(new RStreamerField(newName, GetTypeName())); } std::size_t ROOT::RStreamerField::AppendImpl(const void *from) From 16a90efa1dc15047757ad81e643f5c164f3d20ef Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Sun, 23 Nov 2025 23:44:04 +0100 Subject: [PATCH 5/5] [ntuple] fix streamer field with [U]Long64_t template arg --- tree/ntuple/src/RFieldMeta.cxx | 4 ++++ tree/ntuple/test/rfield_class.cxx | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/tree/ntuple/src/RFieldMeta.cxx b/tree/ntuple/src/RFieldMeta.cxx index 67074b99b7abb..5fdfd99aad56a 100644 --- a/tree/ntuple/src/RFieldMeta.cxx +++ b/tree/ntuple/src/RFieldMeta.cxx @@ -1021,6 +1021,10 @@ ROOT::RStreamerField::RStreamerField(std::string_view fieldName, TClass *classp) fClass(classp), fIndex(0) { + std::string renormalizedAlias; + if (Internal::NeedsMetaNameAsAlias(classp->GetName(), renormalizedAlias)) + fTypeAlias = renormalizedAlias; + fTraits |= kTraitTypeChecksum; // For RClassField, we only check for explicit constructors and destructors and then recursively combine traits from // all member subfields. For RStreamerField, we treat the class as a black box and additionally need to check for diff --git a/tree/ntuple/test/rfield_class.cxx b/tree/ntuple/test/rfield_class.cxx index d6887f45eb17f..ac442000c6aa6 100644 --- a/tree/ntuple/test/rfield_class.cxx +++ b/tree/ntuple/test/rfield_class.cxx @@ -400,6 +400,10 @@ TEST(RNTuple, TClassMetaName) auto f5 = std::make_unique>>("f"); EXPECT_TRUE(dynamic_cast(f5.get())); EXPECT_EQ("StructUsingCollectionProxy", f5->GetTypeAlias()); + + ROOT::RStreamerField f6("f", "EdmWrapper"); + EXPECT_STREQ("EdmWrapper", f6.GetClass()->GetName()); + EXPECT_EQ("EdmWrapper", f6.GetTypeAlias()); } TEST(RNTuple, StreamerInfoRecords)