From b8140ceff5fb873eba42d8e09a5d18f574defd7a Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Wed, 14 May 2025 12:12:32 +0200 Subject: [PATCH 01/11] [ntuple] Do type-check in RValue::GetRef with basic data types Fixes https://github.com/root-project/root/issues/18316 --- tree/ntuple/inc/ROOT/RFieldBase.hxx | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tree/ntuple/inc/ROOT/RFieldBase.hxx b/tree/ntuple/inc/ROOT/RFieldBase.hxx index 6f92760e4b618..0d1bb3234666a 100644 --- a/tree/ntuple/inc/ROOT/RFieldBase.hxx +++ b/tree/ntuple/inc/ROOT/RFieldBase.hxx @@ -27,6 +27,7 @@ #include #include #include +#include namespace ROOT { @@ -727,9 +728,28 @@ public: return std::static_pointer_cast(fObjPtr); } + /** + * Get object reference, already casted to template type + * \note If a the passed template type does not match the field base type name, an exception is thrown + */ template const T &GetRef() const { + const auto templName = typeid(T).name(); + const auto typeName = fField ? fField->GetTypeName() : ""; + if ((std::strncmp(templName, "c", 1) == 0 && typeName != "char") || + (std::strncmp(templName, "h", 1) == 0 && typeName != "unsigned char") || + (std::strncmp(templName, "s", 1) == 0 && typeName != "short") || + (std::strncmp(templName, "t", 1) == 0 && typeName != "unsigned short") || + (std::strncmp(templName, "i", 1) == 0 && typeName != "int") || + (std::strncmp(templName, "j", 1) == 0 && typeName != "unsigned int") || + (std::strncmp(templName, "l", 1) == 0 && typeName != "long") || + (std::strncmp(templName, "m", 1) == 0 && typeName != "unsigned long") || + (std::strncmp(templName, "f", 1) == 0 && typeName != "float") || + (std::strncmp(templName, "d", 1) == 0 && typeName != "double") || + (std::strncmp(templName, "e", 1) == 0 && typeName != "long double")) { + throw RException(R__FAIL("Mismatch between type name `" + typeName + "` and template type `" + templName + "`.")); + } return *static_cast(fObjPtr.get()); } From 5b252028ffcedd8076bcf3911986d4abc60eaf78 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Wed, 14 May 2025 12:33:33 +0200 Subject: [PATCH 02/11] [ntuple][test] add test for thrown exceptions --- tree/ntuple/test/ntuple_view.cxx | 48 ++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tree/ntuple/test/ntuple_view.cxx b/tree/ntuple/test/ntuple_view.cxx index 0af0c959555c5..5c59003399f9f 100644 --- a/tree/ntuple/test/ntuple_view.cxx +++ b/tree/ntuple/test/ntuple_view.cxx @@ -217,6 +217,54 @@ TEST(RNTuple, VoidView) EXPECT_STREQ("pt", viewPt.GetField().GetFieldName().c_str()); } + +TEST(RNTuple, VoidViewThrow) +{ + FileRaii fileGuard("test_ntuple_voidview_throw.root"); + + auto model = RNTupleModel::Create(); + *model->MakeField("c") = 42; + *model->MakeField("h") = 42; + *model->MakeField("s") = 42; + *model->MakeField("t") = 42; + *model->MakeField("i") = 42; + *model->MakeField("j") = 42; + *model->MakeField("l") = 42; + *model->MakeField("m") = 42; + *model->MakeField("f") = 42.f; + *model->MakeField("d") = 42.; + *model->MakeField("e") = 42.; + { + auto writer = RNTupleWriter::Recreate(std::move(model), "ntpl", fileGuard.GetPath()); + writer->Fill(); + } + + auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath()); + EXPECT_EQ(1u, reader->GetNEntries()); + auto vc = reader->GetView("c"); + auto vh = reader->GetView("h"); + auto vs = reader->GetView("s"); + auto vt = reader->GetView("t"); + auto vi = reader->GetView("i"); + auto vj = reader->GetView("j"); + auto vl = reader->GetView("l"); + auto vm = reader->GetView("m"); + auto vf = reader->GetView("f"); + auto vd = reader->GetView("d"); + auto ve = reader->GetView("e"); + EXPECT_THROW(vc.GetValue().GetRef()); + EXPECT_THROW(vh.GetValue().GetRef()); + EXPECT_THROW(vs.GetValue().GetRef()); + EXPECT_THROW(vt.GetValue().GetRef()); + EXPECT_THROW(vi.GetValue().GetRef()); + EXPECT_THROW(vj.GetValue().GetRef()); + EXPECT_THROW(vl.GetValue().GetRef()); + EXPECT_THROW(vm.GetValue().GetRef()); + EXPECT_THROW(vf.GetValue().GetRef()); + EXPECT_THROW(vd.GetValue().GetRef()); + EXPECT_THROW(ve.GetValue().GetRef()); +} + TEST(RNTuple, MissingViewNames) { FileRaii fileGuard("test_ntuple_missing_view_names.root"); From ffe6ae71b52bbbf4570bce2a4fc3e36d8e62a445 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Wed, 14 May 2025 12:37:47 +0200 Subject: [PATCH 03/11] [nfc] clangformat --- tree/ntuple/inc/ROOT/RFieldBase.hxx | 3 ++- tree/ntuple/test/ntuple_view.cxx | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RFieldBase.hxx b/tree/ntuple/inc/ROOT/RFieldBase.hxx index 0d1bb3234666a..4bce3f3a305c8 100644 --- a/tree/ntuple/inc/ROOT/RFieldBase.hxx +++ b/tree/ntuple/inc/ROOT/RFieldBase.hxx @@ -748,7 +748,8 @@ public: (std::strncmp(templName, "f", 1) == 0 && typeName != "float") || (std::strncmp(templName, "d", 1) == 0 && typeName != "double") || (std::strncmp(templName, "e", 1) == 0 && typeName != "long double")) { - throw RException(R__FAIL("Mismatch between type name `" + typeName + "` and template type `" + templName + "`.")); + throw RException( + R__FAIL("Mismatch between type name `" + typeName + "` and template type `" + templName + "`.")); } return *static_cast(fObjPtr.get()); } diff --git a/tree/ntuple/test/ntuple_view.cxx b/tree/ntuple/test/ntuple_view.cxx index 5c59003399f9f..304ad9b20d3af 100644 --- a/tree/ntuple/test/ntuple_view.cxx +++ b/tree/ntuple/test/ntuple_view.cxx @@ -217,7 +217,6 @@ TEST(RNTuple, VoidView) EXPECT_STREQ("pt", viewPt.GetField().GetFieldName().c_str()); } - TEST(RNTuple, VoidViewThrow) { FileRaii fileGuard("test_ntuple_voidview_throw.root"); From af64af5a3568de88ff488416640786ee53037529 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Wed, 14 May 2025 12:57:59 +0200 Subject: [PATCH 04/11] [ntuple][test] fix compilation error and add more test cases --- tree/ntuple/test/ntuple_view.cxx | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/tree/ntuple/test/ntuple_view.cxx b/tree/ntuple/test/ntuple_view.cxx index 304ad9b20d3af..1dfb5f58e876e 100644 --- a/tree/ntuple/test/ntuple_view.cxx +++ b/tree/ntuple/test/ntuple_view.cxx @@ -251,17 +251,21 @@ TEST(RNTuple, VoidViewThrow) auto vf = reader->GetView("f"); auto vd = reader->GetView("d"); auto ve = reader->GetView("e"); - EXPECT_THROW(vc.GetValue().GetRef()); - EXPECT_THROW(vh.GetValue().GetRef()); - EXPECT_THROW(vs.GetValue().GetRef()); - EXPECT_THROW(vt.GetValue().GetRef()); - EXPECT_THROW(vi.GetValue().GetRef()); - EXPECT_THROW(vj.GetValue().GetRef()); - EXPECT_THROW(vl.GetValue().GetRef()); - EXPECT_THROW(vm.GetValue().GetRef()); - EXPECT_THROW(vf.GetValue().GetRef()); - EXPECT_THROW(vd.GetValue().GetRef()); - EXPECT_THROW(ve.GetValue().GetRef()); + EXPECT_THROW(vc.GetValue().GetRef(), ROOT::RException); + EXPECT_THROW(vh.GetValue().GetRef(), ROOT::RException); + EXPECT_THROW(vs.GetValue().GetRef(), ROOT::RException); + EXPECT_THROW(vt.GetValue().GetRef(), ROOT::RException); + EXPECT_THROW(vi.GetValue().GetRef(), ROOT::RException); + EXPECT_THROW(vj.GetValue().GetRef(), ROOT::RException); + EXPECT_THROW(vl.GetValue().GetRef(), ROOT::RException); + EXPECT_THROW(vm.GetValue().GetRef(), ROOT::RException); + EXPECT_THROW(vf.GetValue().GetRef(), ROOT::RException); + EXPECT_THROW(vf.GetValue().GetRef(), ROOT::RException); + EXPECT_THROW(vd.GetValue().GetRef(), ROOT::RException); + EXPECT_THROW(vd.GetValue().GetRef(), ROOT::RException); + EXPECT_THROW(ve.GetValue().GetRef(), ROOT::RException); + EXPECT_FLOAT_EQ(vf.GetValue().GetRef(), 42.f); + EXPECT_FLOAT_EQ(vd.GetValue().GetRef(), 42.); } TEST(RNTuple, MissingViewNames) From 7707f30c320d877af530586f5c5b9c7f26852d81 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Wed, 14 May 2025 13:08:51 +0200 Subject: [PATCH 05/11] [ntuple] do not create long double field --- tree/ntuple/test/ntuple_view.cxx | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tree/ntuple/test/ntuple_view.cxx b/tree/ntuple/test/ntuple_view.cxx index 1dfb5f58e876e..375da06168a37 100644 --- a/tree/ntuple/test/ntuple_view.cxx +++ b/tree/ntuple/test/ntuple_view.cxx @@ -232,7 +232,6 @@ TEST(RNTuple, VoidViewThrow) *model->MakeField("m") = 42; *model->MakeField("f") = 42.f; *model->MakeField("d") = 42.; - *model->MakeField("e") = 42.; { auto writer = RNTupleWriter::Recreate(std::move(model), "ntpl", fileGuard.GetPath()); writer->Fill(); @@ -250,7 +249,6 @@ TEST(RNTuple, VoidViewThrow) auto vm = reader->GetView("m"); auto vf = reader->GetView("f"); auto vd = reader->GetView("d"); - auto ve = reader->GetView("e"); EXPECT_THROW(vc.GetValue().GetRef(), ROOT::RException); EXPECT_THROW(vh.GetValue().GetRef(), ROOT::RException); EXPECT_THROW(vs.GetValue().GetRef(), ROOT::RException); @@ -263,7 +261,7 @@ TEST(RNTuple, VoidViewThrow) EXPECT_THROW(vf.GetValue().GetRef(), ROOT::RException); EXPECT_THROW(vd.GetValue().GetRef(), ROOT::RException); EXPECT_THROW(vd.GetValue().GetRef(), ROOT::RException); - EXPECT_THROW(ve.GetValue().GetRef(), ROOT::RException); + EXPECT_THROW(vd.GetValue().GetRef(), ROOT::RException); EXPECT_FLOAT_EQ(vf.GetValue().GetRef(), 42.f); EXPECT_FLOAT_EQ(vd.GetValue().GetRef(), 42.); } From 305bcbffdcfde0324f3941db2d732e08bf858d31 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Wed, 14 May 2025 14:12:37 +0200 Subject: [PATCH 06/11] [ntuple] deal with secondary type names --- tree/ntuple/inc/ROOT/RFieldBase.hxx | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RFieldBase.hxx b/tree/ntuple/inc/ROOT/RFieldBase.hxx index 4bce3f3a305c8..312ef38543eec 100644 --- a/tree/ntuple/inc/ROOT/RFieldBase.hxx +++ b/tree/ntuple/inc/ROOT/RFieldBase.hxx @@ -736,15 +736,17 @@ public: const T &GetRef() const { const auto templName = typeid(T).name(); - const auto typeName = fField ? fField->GetTypeName() : ""; - if ((std::strncmp(templName, "c", 1) == 0 && typeName != "char") || - (std::strncmp(templName, "h", 1) == 0 && typeName != "unsigned char") || - (std::strncmp(templName, "s", 1) == 0 && typeName != "short") || - (std::strncmp(templName, "t", 1) == 0 && typeName != "unsigned short") || - (std::strncmp(templName, "i", 1) == 0 && typeName != "int") || - (std::strncmp(templName, "j", 1) == 0 && typeName != "unsigned int") || - (std::strncmp(templName, "l", 1) == 0 && typeName != "long") || - (std::strncmp(templName, "m", 1) == 0 && typeName != "unsigned long") || + auto typeName = fField ? fField->GetTypeName() : ""; + if (typeName.substr(0, 25) == "ROOT::RNTupleCardinality<") + typeName = ROOT::Internal::GetCanonicalTypePrefix(ROOT::Internal::TokenizeTypeList(typeName.substr(25, typeName.length() - 26)).at(0)); + if ((std::strncmp(templName, "c", 1) == 0 && typeName != "char" && typeName != "std::int8_t") || + (std::strncmp(templName, "h", 1) == 0 && typeName != "unsigned char" && typeName != "std::uint8_t" && typeName != "std::byte") || + (std::strncmp(templName, "s", 1) == 0 && typeName != "short" && typeName != "std::int16_t") || + (std::strncmp(templName, "t", 1) == 0 && typeName != "unsigned short" && typeName != "std::uint16_t") || + (std::strncmp(templName, "i", 1) == 0 && typeName != "int" && typeName != "std::int32_t") || + (std::strncmp(templName, "j", 1) == 0 && typeName != "unsigned int" && typeName != "std::uint32_t") || + (std::strncmp(templName, "l", 1) == 0 && typeName != "long" && typeName != "std::int64_t") || + (std::strncmp(templName, "m", 1) == 0 && typeName != "unsigned long" && typeName != "std::int64_t" && typeName != "ROOT::RNTupleCardinality") || (std::strncmp(templName, "f", 1) == 0 && typeName != "float") || (std::strncmp(templName, "d", 1) == 0 && typeName != "double") || (std::strncmp(templName, "e", 1) == 0 && typeName != "long double")) { From 961cd3815685c202abdacf08b199c95a85001d97 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Wed, 14 May 2025 14:39:47 +0200 Subject: [PATCH 07/11] [ntuple] skip for now --- tree/ntuple/inc/ROOT/RFieldBase.hxx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RFieldBase.hxx b/tree/ntuple/inc/ROOT/RFieldBase.hxx index 312ef38543eec..708706c9bbb4e 100644 --- a/tree/ntuple/inc/ROOT/RFieldBase.hxx +++ b/tree/ntuple/inc/ROOT/RFieldBase.hxx @@ -737,8 +737,8 @@ public: { const auto templName = typeid(T).name(); auto typeName = fField ? fField->GetTypeName() : ""; - if (typeName.substr(0, 25) == "ROOT::RNTupleCardinality<") - typeName = ROOT::Internal::GetCanonicalTypePrefix(ROOT::Internal::TokenizeTypeList(typeName.substr(25, typeName.length() - 26)).at(0)); + // if (typeName.substr(0, 25) == "ROOT::RNTupleCardinality<") + // typeName = ROOT::Internal::GetCanonicalTypePrefix(ROOT::Internal::TokenizeTypeList(typeName.substr(25, typeName.length() - 26)).at(0)); if ((std::strncmp(templName, "c", 1) == 0 && typeName != "char" && typeName != "std::int8_t") || (std::strncmp(templName, "h", 1) == 0 && typeName != "unsigned char" && typeName != "std::uint8_t" && typeName != "std::byte") || (std::strncmp(templName, "s", 1) == 0 && typeName != "short" && typeName != "std::int16_t") || From a565732a615138e73f7e57a314501dea9b345651 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Wed, 14 May 2025 15:18:31 +0200 Subject: [PATCH 08/11] [ntuple] fix test failures --- tree/ntuple/inc/ROOT/RFieldBase.hxx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RFieldBase.hxx b/tree/ntuple/inc/ROOT/RFieldBase.hxx index 708706c9bbb4e..150cf01bf6e72 100644 --- a/tree/ntuple/inc/ROOT/RFieldBase.hxx +++ b/tree/ntuple/inc/ROOT/RFieldBase.hxx @@ -744,9 +744,9 @@ public: (std::strncmp(templName, "s", 1) == 0 && typeName != "short" && typeName != "std::int16_t") || (std::strncmp(templName, "t", 1) == 0 && typeName != "unsigned short" && typeName != "std::uint16_t") || (std::strncmp(templName, "i", 1) == 0 && typeName != "int" && typeName != "std::int32_t") || - (std::strncmp(templName, "j", 1) == 0 && typeName != "unsigned int" && typeName != "std::uint32_t") || + (std::strncmp(templName, "j", 1) == 0 && typeName != "unsigned int" && typeName != "std::uint32_t" && typeName != "ROOT::RNTupleCardinality) || (std::strncmp(templName, "l", 1) == 0 && typeName != "long" && typeName != "std::int64_t") || - (std::strncmp(templName, "m", 1) == 0 && typeName != "unsigned long" && typeName != "std::int64_t" && typeName != "ROOT::RNTupleCardinality") || + (std::strncmp(templName, "m", 1) == 0 && typeName != "long unsigned int" && typeName != "std::int64_t" && typeName != "ROOT::RNTupleCardinality") || (std::strncmp(templName, "f", 1) == 0 && typeName != "float") || (std::strncmp(templName, "d", 1) == 0 && typeName != "double") || (std::strncmp(templName, "e", 1) == 0 && typeName != "long double")) { From 9cc3edc99a9a35ab1307f04631c13088aa955a55 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Wed, 14 May 2025 15:25:03 +0200 Subject: [PATCH 09/11] [ntuple] do right fix --- tree/ntuple/inc/ROOT/RFieldBase.hxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tree/ntuple/inc/ROOT/RFieldBase.hxx b/tree/ntuple/inc/ROOT/RFieldBase.hxx index 150cf01bf6e72..80d655ef65296 100644 --- a/tree/ntuple/inc/ROOT/RFieldBase.hxx +++ b/tree/ntuple/inc/ROOT/RFieldBase.hxx @@ -746,7 +746,7 @@ public: (std::strncmp(templName, "i", 1) == 0 && typeName != "int" && typeName != "std::int32_t") || (std::strncmp(templName, "j", 1) == 0 && typeName != "unsigned int" && typeName != "std::uint32_t" && typeName != "ROOT::RNTupleCardinality) || (std::strncmp(templName, "l", 1) == 0 && typeName != "long" && typeName != "std::int64_t") || - (std::strncmp(templName, "m", 1) == 0 && typeName != "long unsigned int" && typeName != "std::int64_t" && typeName != "ROOT::RNTupleCardinality") || + (std::strncmp(templName, "m", 1) == 0 && typeName != "unsigned long" && typeName != "std::uint64_t" && typeName != "ROOT::RNTupleCardinality") || (std::strncmp(templName, "f", 1) == 0 && typeName != "float") || (std::strncmp(templName, "d", 1) == 0 && typeName != "double") || (std::strncmp(templName, "e", 1) == 0 && typeName != "long double")) { From 4aecd1a9acd74bc233526abf6fb991115190dd9a Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Wed, 14 May 2025 15:47:21 +0200 Subject: [PATCH 10/11] [ntuple] fix typo --- tree/ntuple/inc/ROOT/RFieldBase.hxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tree/ntuple/inc/ROOT/RFieldBase.hxx b/tree/ntuple/inc/ROOT/RFieldBase.hxx index 80d655ef65296..2f1621b4ed3de 100644 --- a/tree/ntuple/inc/ROOT/RFieldBase.hxx +++ b/tree/ntuple/inc/ROOT/RFieldBase.hxx @@ -744,7 +744,7 @@ public: (std::strncmp(templName, "s", 1) == 0 && typeName != "short" && typeName != "std::int16_t") || (std::strncmp(templName, "t", 1) == 0 && typeName != "unsigned short" && typeName != "std::uint16_t") || (std::strncmp(templName, "i", 1) == 0 && typeName != "int" && typeName != "std::int32_t") || - (std::strncmp(templName, "j", 1) == 0 && typeName != "unsigned int" && typeName != "std::uint32_t" && typeName != "ROOT::RNTupleCardinality) || + (std::strncmp(templName, "j", 1) == 0 && typeName != "unsigned int" && typeName != "std::uint32_t" && typeName != "ROOT::RNTupleCardinality") || (std::strncmp(templName, "l", 1) == 0 && typeName != "long" && typeName != "std::int64_t") || (std::strncmp(templName, "m", 1) == 0 && typeName != "unsigned long" && typeName != "std::uint64_t" && typeName != "ROOT::RNTupleCardinality") || (std::strncmp(templName, "f", 1) == 0 && typeName != "float") || From d186288ecc0c3dffd52427f5d5df25a5b0ac56dd Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Wed, 14 May 2025 16:31:55 +0200 Subject: [PATCH 11/11] [ntuple][test] fix entry number --- tree/ntuple/test/ntuple_view.cxx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tree/ntuple/test/ntuple_view.cxx b/tree/ntuple/test/ntuple_view.cxx index 375da06168a37..88b4204ffed6f 100644 --- a/tree/ntuple/test/ntuple_view.cxx +++ b/tree/ntuple/test/ntuple_view.cxx @@ -262,7 +262,9 @@ TEST(RNTuple, VoidViewThrow) EXPECT_THROW(vd.GetValue().GetRef(), ROOT::RException); EXPECT_THROW(vd.GetValue().GetRef(), ROOT::RException); EXPECT_THROW(vd.GetValue().GetRef(), ROOT::RException); + vf(0); EXPECT_FLOAT_EQ(vf.GetValue().GetRef(), 42.f); + vd(0); EXPECT_FLOAT_EQ(vd.GetValue().GetRef(), 42.); }