From 142ed8d47d379d219184642afdd393ffb8ece387 Mon Sep 17 00:00:00 2001 From: Andrew Olivier Date: Thu, 3 Sep 2026 14:50:18 -0500 Subject: [PATCH 01/11] Transitioned FORM's RNTuple writing backend from public data members in its association container to accessor functions. The RNTuple association container is now responsible for making sure the shared RNTupleWriter only gets created once. --- .../root_rfield_write_container.cpp | 48 +++++------------ .../root_rfield_write_container.hpp | 1 - .../root_rntuple_write_container.cpp | 45 ++++++++++++++-- .../root_rntuple_write_container.hpp | 52 ++++++++++++------- 4 files changed, 88 insertions(+), 58 deletions(-) diff --git a/form/root_storage/root_rfield_write_container.cpp b/form/root_storage/root_rfield_write_container.cpp index 4c456130d..bccf46b2d 100644 --- a/form/root_storage/root_rfield_write_container.cpp +++ b/form/root_storage/root_rfield_write_container.cpp @@ -30,22 +30,12 @@ namespace form::detail::experimental { void root_rfield_write_container_imp::set_file(std::shared_ptr file) { - storage_write_container::set_file(file); - - auto form_root_file = dynamic_pointer_cast(file); - if (form_root_file) { - tfile_ = form_root_file->get_tfile(); - } else { - throw std::runtime_error("root_rfield_write_container_imp::set_file failed to convert an " - "i_storage_file to a root_tfile_imp. " - "root_rfield_write_container_imp only works with TFiles."); - } - - if (!tfile_) { - throw std::runtime_error( - "root_rfield_write_container_imp::set_file failed to get a TFile from a root_tfile_imp"); + //The test below is required by FORM's testing infrastructure + if(!dynamic_pointer_cast(file)) { + throw std::runtime_error("root_rfield_write_container_imp::set_file was passed a file that is not a ROOT file."); } + storage_write_container::set_file(file); return; } @@ -69,22 +59,12 @@ namespace form::detail::experimental { "root_rfield_write_container_imp::fill No parent RNTuple set up before first fill() call"); } - if (!rntuple_parent_->writer) { - if (!tfile_) { - throw std::runtime_error( - "root_rfield_write_container_imp::fill No file loaded to write to on first fill() call"); - } - - rntuple_parent_->writer = - ROOT::RNTupleWriter::Append(std::move(rntuple_parent_->model), top_name(), *tfile_); - rntuple_parent_->entry = rntuple_parent_->writer->CreateRawPtrWriteEntry(); - } - rntuple_parent_->entry->BindRawPtr(col_name(), data); + rntuple_parent_->get_entry().BindRawPtr(col_name(), data); // Unlike a TBranch, an RNTuple entry is only written on commit(); // every field bound before that commit shares one entry. // Return the 0-based index that pending entry will occupy (the current entry count). - return static_cast(rntuple_parent_->writer->GetNEntries()); + return static_cast(rntuple_parent_->get_writer().GetNEntries()); } void root_rfield_write_container_imp::commit() @@ -94,13 +74,14 @@ namespace form::detail::experimental { "You may have called commit() without calling set_parent() first."); } - if (!rntuple_parent_->entry) { - throw std::runtime_error( - "root_rfield_write_container_imp::commit No RRawPtrWriteEntry set up. " - "You may have called commit() without calling fill() first."); + //If get_model() is not nullptr, then root_rntuple_write_container_imp guarantees that get_writer() + //has not yet been run. + if (rntuple_parent_->get_model()) { + throw std::runtime_error("root_rfield_write_container_imp::commit No RNTupleWriter set up. " + "You may have called commit() without calling setup_write() first."); } - assert(rntuple_parent_->writer); // writer and entry are set in the same place: fill() - rntuple_parent_->writer->Fill(*rntuple_parent_->entry); + + rntuple_parent_->get_writer().Fill(rntuple_parent_->get_entry()); } //setup_write() may not be called after the first time fill() is called. @@ -134,7 +115,6 @@ namespace form::detail::experimental { } } - rntuple_parent_->model->AddField(std::move(field)); + rntuple_parent_->get_model()->AddField(std::move(field)); } - } diff --git a/form/root_storage/root_rfield_write_container.hpp b/form/root_storage/root_rfield_write_container.hpp index d0c85bea3..9dec4d42e 100644 --- a/form/root_storage/root_rfield_write_container.hpp +++ b/form/root_storage/root_rfield_write_container.hpp @@ -30,7 +30,6 @@ namespace form::detail::experimental { void commit() override; private: - std::shared_ptr tfile_; std::shared_ptr rntuple_parent_; bool force_streamer_field_ = false; diff --git a/form/root_storage/root_rntuple_write_container.cpp b/form/root_storage/root_rntuple_write_container.cpp index af5e682fe..cc40ae657 100644 --- a/form/root_storage/root_rntuple_write_container.cpp +++ b/form/root_storage/root_rntuple_write_container.cpp @@ -12,20 +12,34 @@ namespace form::detail::experimental { root_rntuple_write_container_imp::root_rntuple_write_container_imp(std::string const& name) : - storage_write_association(name), model(ROOT::RNTupleModel::Create()) + storage_write_association(name), model_(ROOT::RNTupleModel::Create()) { } root_rntuple_write_container_imp::~root_rntuple_write_container_imp() { - if (writer) { - writer->CommitDataset(); + if (writer_) { + writer_->CommitDataset(); } } void root_rntuple_write_container_imp::set_file(std::shared_ptr file) { storage_write_container::set_file(file); + + auto form_root_file = dynamic_pointer_cast(file); + if (form_root_file) { + tfile_ = form_root_file->get_tfile(); + } else { + throw std::runtime_error("root_rntuple_write_container_imp::set_file failed to convert an " + "i_storage_file to a root_tfile_imp. " + "root_rntuple_write_container_imp only works with TFiles."); + } + + if (!tfile_) { + throw std::runtime_error( + "root_rntuple_write_container_imp::set_file failed to get a TFile from a root_tfile_imp"); + } return; } @@ -40,4 +54,29 @@ namespace form::detail::experimental { } void root_rntuple_write_container_imp::setup_write(std::type_info const& /*type*/) { return; } + + ROOT::RNTupleWriter& root_rntuple_write_container_imp::get_writer() + { + if(!tfile_) throw std::runtime_error("root_rntuple_write_container_imp::setup_write no file loaded to write to on first fill() call"); + if(!writer_) + { + writer_ = ROOT::RNTupleWriter::Append(std::move(model_), name(), *tfile_); + } + + return *writer_; + } + + std::unique_ptr const& root_rntuple_write_container_imp::get_model() + { + return model_; + } + + RRawPtrWriteEntry& root_rntuple_write_container_imp::get_entry() + { + if(!entry_) + { + entry_ = get_writer().CreateRawPtrWriteEntry(); + } + return *entry_; + } } diff --git a/form/root_storage/root_rntuple_write_container.hpp b/form/root_storage/root_rntuple_write_container.hpp index 310f16f61..f381290cf 100644 --- a/form/root_storage/root_rntuple_write_container.hpp +++ b/form/root_storage/root_rntuple_write_container.hpp @@ -43,26 +43,38 @@ namespace form::detail::experimental { using RRawPtrWriteEntry = ROOT::Experimental::Detail::RRawPtrWriteEntry; #endif - struct root_rntuple_write_container_imp : public storage_write_association { - root_rntuple_write_container_imp(std::string const& name); - ~root_rntuple_write_container_imp() override; - - //Rule of five - root_rntuple_write_container_imp(root_rntuple_write_container_imp const& other) = delete; - root_rntuple_write_container_imp(root_rntuple_write_container_imp&& other) = delete; - root_rntuple_write_container_imp& operator=(root_rntuple_write_container_imp const& other) = - delete; - root_rntuple_write_container_imp& operator=(root_rntuple_write_container_imp&& other) = delete; - - void set_file(std::shared_ptr file) override; - void setup_write(std::type_info const& type) override; - std::uint64_t fill(void const* data) override; - void commit() override; - - //State shared by root_rfield_write_container_imps - std::unique_ptr writer; - std::unique_ptr model; - std::unique_ptr entry; + class root_rntuple_write_container_imp : public storage_write_association { + public: + root_rntuple_write_container_imp(std::string const& name); + ~root_rntuple_write_container_imp() override; + + //Rule of five + root_rntuple_write_container_imp(root_rntuple_write_container_imp const& other) = delete; + root_rntuple_write_container_imp(root_rntuple_write_container_imp&& other) = delete; + root_rntuple_write_container_imp& operator=(root_rntuple_write_container_imp const& other) = + delete; + root_rntuple_write_container_imp& operator=(root_rntuple_write_container_imp&& other) = delete; + + void set_file(std::shared_ptr file) override; + void setup_write(std::type_info const& type) override; + std::uint64_t fill(void const* data) override; + void commit() override; + + ROOT::RNTupleWriter& get_writer(); + //get_model() also signals whether model_ has already been moved from. + //If model_ has been moved from, the c++ standard guarantees it will contain nullptr. + //This is important for this RNTuple backend to meet FORM's testing + //requirement that commit() shall fail if fill() has not been called yet. + std::unique_ptr const& get_model(); + RRawPtrWriteEntry& get_entry(); + + private: + std::shared_ptr tfile_; + + //State shared by root_rfield_write_container_imps + std::unique_ptr writer_; + std::unique_ptr model_; + std::unique_ptr entry_; }; } From f040fe3f53fe36b3d5a40b81ffe463cb3da51d90 Mon Sep 17 00:00:00 2001 From: Andrew Olivier Date: Thu, 3 Sep 2026 15:41:06 -0500 Subject: [PATCH 02/11] Made `get_model()` const. --- form/root_storage/root_rntuple_write_container.cpp | 2 +- form/root_storage/root_rntuple_write_container.hpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/form/root_storage/root_rntuple_write_container.cpp b/form/root_storage/root_rntuple_write_container.cpp index cc40ae657..42faff29b 100644 --- a/form/root_storage/root_rntuple_write_container.cpp +++ b/form/root_storage/root_rntuple_write_container.cpp @@ -66,7 +66,7 @@ namespace form::detail::experimental { return *writer_; } - std::unique_ptr const& root_rntuple_write_container_imp::get_model() + std::unique_ptr const& root_rntuple_write_container_imp::get_model() const { return model_; } diff --git a/form/root_storage/root_rntuple_write_container.hpp b/form/root_storage/root_rntuple_write_container.hpp index f381290cf..90f57b210 100644 --- a/form/root_storage/root_rntuple_write_container.hpp +++ b/form/root_storage/root_rntuple_write_container.hpp @@ -65,7 +65,7 @@ namespace form::detail::experimental { //If model_ has been moved from, the c++ standard guarantees it will contain nullptr. //This is important for this RNTuple backend to meet FORM's testing //requirement that commit() shall fail if fill() has not been called yet. - std::unique_ptr const& get_model(); + std::unique_ptr const& get_model() const; RRawPtrWriteEntry& get_entry(); private: From 161733d1b0d2d7adcf28e2368c637fbb2671608c Mon Sep 17 00:00:00 2001 From: Andrew Olivier Date: Thu, 3 Sep 2026 15:46:33 -0500 Subject: [PATCH 03/11] Added assert() where RField write container adds another field to the RNTuple model. This documents a contract between FORM and the RNTuple backend that FORM shall finish all setup_write() calls before the first time it calls fill(). --- form/root_storage/root_rfield_write_container.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/form/root_storage/root_rfield_write_container.cpp b/form/root_storage/root_rfield_write_container.cpp index bccf46b2d..4c2358927 100644 --- a/form/root_storage/root_rfield_write_container.cpp +++ b/form/root_storage/root_rfield_write_container.cpp @@ -115,6 +115,7 @@ namespace form::detail::experimental { } } + assert(rntuple_parent->get_model()); rntuple_parent_->get_model()->AddField(std::move(field)); } } From 0b9576cde874487c3e045ac7f5bbd989d3aa2f96 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 9 Sep 2026 20:11:43 +0000 Subject: [PATCH 04/11] Apply clang-format fixes --- form/root_storage/root_rfield_write_container.cpp | 5 +++-- form/root_storage/root_rntuple_write_container.cpp | 10 +++++----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/form/root_storage/root_rfield_write_container.cpp b/form/root_storage/root_rfield_write_container.cpp index c99de8217..aa3638b3a 100644 --- a/form/root_storage/root_rfield_write_container.cpp +++ b/form/root_storage/root_rfield_write_container.cpp @@ -31,8 +31,9 @@ namespace form::detail::experimental { void root_rfield_write_container_imp::set_file(std::shared_ptr file) { //The test below is required by FORM's testing infrastructure - if(!dynamic_pointer_cast(file)) { - throw std::runtime_error("root_rfield_write_container_imp::set_file was passed a file that is not a ROOT file."); + if (!dynamic_pointer_cast(file)) { + throw std::runtime_error( + "root_rfield_write_container_imp::set_file was passed a file that is not a ROOT file."); } storage_write_container::set_file(file); diff --git a/form/root_storage/root_rntuple_write_container.cpp b/form/root_storage/root_rntuple_write_container.cpp index 6ca04beac..574b6d397 100644 --- a/form/root_storage/root_rntuple_write_container.cpp +++ b/form/root_storage/root_rntuple_write_container.cpp @@ -55,9 +55,10 @@ namespace form::detail::experimental { ROOT::RNTupleWriter& root_rntuple_write_container_imp::get_writer() { - if(!tfile_) throw std::runtime_error("root_rntuple_write_container_imp::setup_write no file loaded to write to on first fill() call"); - if(!writer_) - { + if (!tfile_) + throw std::runtime_error("root_rntuple_write_container_imp::setup_write no file loaded to " + "write to on first fill() call"); + if (!writer_) { writer_ = ROOT::RNTupleWriter::Append(std::move(model_), name(), *tfile_); } @@ -71,8 +72,7 @@ namespace form::detail::experimental { RRawPtrWriteEntry& root_rntuple_write_container_imp::get_entry() { - if(!entry_) - { + if (!entry_) { entry_ = get_writer().CreateRawPtrWriteEntry(); } return *entry_; From 9995a0228c26a2be657707019e147348b28fc14d Mon Sep 17 00:00:00 2001 From: Andrew Olivier Date: Wed, 9 Sep 2026 15:22:30 -0500 Subject: [PATCH 05/11] Fix assert() condition that didn't compile in debug mode. --- form/root_storage/root_rfield_write_container.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/form/root_storage/root_rfield_write_container.cpp b/form/root_storage/root_rfield_write_container.cpp index aa3638b3a..6865b81a5 100644 --- a/form/root_storage/root_rfield_write_container.cpp +++ b/form/root_storage/root_rfield_write_container.cpp @@ -116,7 +116,7 @@ namespace form::detail::experimental { } } - assert(rntuple_parent->get_model()); + assert(rntuple_parent_->get_model()); rntuple_parent_->get_model()->AddField(std::move(field)); } } From 114f47c13346b23a265b8283a17bbf31578e275c Mon Sep 17 00:00:00 2001 From: Andrew Olivier Date: Wed, 9 Sep 2026 15:27:18 -0500 Subject: [PATCH 06/11] Catch exception from CommitDataset() at code rabbit's suggestion. --- form/root_storage/root_rntuple_write_container.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/form/root_storage/root_rntuple_write_container.cpp b/form/root_storage/root_rntuple_write_container.cpp index 574b6d397..ea1d6624e 100644 --- a/form/root_storage/root_rntuple_write_container.cpp +++ b/form/root_storage/root_rntuple_write_container.cpp @@ -19,7 +19,11 @@ namespace form::detail::experimental { root_rntuple_write_container_imp::~root_rntuple_write_container_imp() { if (writer_) { - writer_->CommitDataset(); + try { + writer_->CommitDataset(); + } catch(ROOT::RException const& e) { + std::cerr << "Failed to commit RNTuple " << name() << " at destruction." << std::endl; + } } } From b921b3ad4674f82e0c26a2186e8f294a21fe18de Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 9 Sep 2026 20:32:47 +0000 Subject: [PATCH 07/11] Apply clang-format fixes --- form/root_storage/root_rntuple_write_container.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/form/root_storage/root_rntuple_write_container.cpp b/form/root_storage/root_rntuple_write_container.cpp index ea1d6624e..dbc19d48b 100644 --- a/form/root_storage/root_rntuple_write_container.cpp +++ b/form/root_storage/root_rntuple_write_container.cpp @@ -21,7 +21,7 @@ namespace form::detail::experimental { if (writer_) { try { writer_->CommitDataset(); - } catch(ROOT::RException const& e) { + } catch (ROOT::RException const& e) { std::cerr << "Failed to commit RNTuple " << name() << " at destruction." << std::endl; } } From 17b1e6756d1fcf139f9c07ed477aa0d538d245eb Mon Sep 17 00:00:00 2001 From: Andrew Olivier Date: Wed, 9 Sep 2026 16:25:48 -0500 Subject: [PATCH 08/11] Address clang-tidy comments --- form/root_storage/root_rfield_write_container.cpp | 1 - form/root_storage/root_rntuple_write_container.cpp | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/form/root_storage/root_rfield_write_container.cpp b/form/root_storage/root_rfield_write_container.cpp index 6865b81a5..c058ab25a 100644 --- a/form/root_storage/root_rfield_write_container.cpp +++ b/form/root_storage/root_rfield_write_container.cpp @@ -37,7 +37,6 @@ namespace form::detail::experimental { } storage_write_container::set_file(file); - return; } void root_rfield_write_container_imp::set_parent( diff --git a/form/root_storage/root_rntuple_write_container.cpp b/form/root_storage/root_rntuple_write_container.cpp index ea1d6624e..7cbe7e423 100644 --- a/form/root_storage/root_rntuple_write_container.cpp +++ b/form/root_storage/root_rntuple_write_container.cpp @@ -22,7 +22,7 @@ namespace form::detail::experimental { try { writer_->CommitDataset(); } catch(ROOT::RException const& e) { - std::cerr << "Failed to commit RNTuple " << name() << " at destruction." << std::endl; + std::cerr << "Failed to commit RNTuple " << name() << " at destruction.\n"; } } } @@ -44,7 +44,6 @@ namespace form::detail::experimental { throw std::runtime_error( "root_rntuple_write_container_imp::set_file failed to get a TFile from a root_tfile_imp"); } - return; } std::uint64_t root_rntuple_write_container_imp::fill(void const* /*data*/) From 80595edb8c3b6e6215cef84d32a680a1d84cd882 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 9 Sep 2026 21:30:47 +0000 Subject: [PATCH 09/11] Apply clang-format fixes --- form/root_storage/root_rntuple_write_container.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/form/root_storage/root_rntuple_write_container.cpp b/form/root_storage/root_rntuple_write_container.cpp index 7cbe7e423..ff44ab640 100644 --- a/form/root_storage/root_rntuple_write_container.cpp +++ b/form/root_storage/root_rntuple_write_container.cpp @@ -21,7 +21,7 @@ namespace form::detail::experimental { if (writer_) { try { writer_->CommitDataset(); - } catch(ROOT::RException const& e) { + } catch (ROOT::RException const& e) { std::cerr << "Failed to commit RNTuple " << name() << " at destruction.\n"; } } From dc7970ac5c883f809b44be5fd782bdacef5ea09d Mon Sep 17 00:00:00 2001 From: Andrew Olivier Date: Wed, 9 Sep 2026 16:53:48 -0500 Subject: [PATCH 10/11] Improve code coverage by adding a test and matching TTree backend's standard for checking file format. --- form/root_storage/root_rntuple_write_container.cpp | 11 +++-------- test/form/form_storage_test.cpp | 7 +++++++ 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/form/root_storage/root_rntuple_write_container.cpp b/form/root_storage/root_rntuple_write_container.cpp index 7cbe7e423..3dff94215 100644 --- a/form/root_storage/root_rntuple_write_container.cpp +++ b/form/root_storage/root_rntuple_write_container.cpp @@ -39,11 +39,6 @@ namespace form::detail::experimental { "i_storage_file to a root_tfile_imp. " "root_rntuple_write_container_imp only works with TFiles."); } - - if (!tfile_) { - throw std::runtime_error( - "root_rntuple_write_container_imp::set_file failed to get a TFile from a root_tfile_imp"); - } } std::uint64_t root_rntuple_write_container_imp::fill(void const* /*data*/) @@ -58,10 +53,10 @@ namespace form::detail::experimental { ROOT::RNTupleWriter& root_rntuple_write_container_imp::get_writer() { - if (!tfile_) - throw std::runtime_error("root_rntuple_write_container_imp::setup_write no file loaded to " - "write to on first fill() call"); if (!writer_) { + if (!tfile_) + throw std::runtime_error("root_rntuple_write_container_imp::setup_write no file loaded to " + "write to on first fill() call"); writer_ = ROOT::RNTupleWriter::Append(std::move(model_), name(), *tfile_); } diff --git a/test/form/form_storage_test.cpp b/test/form/form_storage_test.cpp index 21a7f3c49..6db670bc4 100644 --- a/test/form/form_storage_test.cpp +++ b/test/form/form_storage_test.cpp @@ -164,6 +164,13 @@ TEST_CASE("FORM Container setup error handling") write_container->setup_write(type_info); CHECK_THROWS_AS(write_container->commit(), std::runtime_error); } + + SECTION("set_file() on parent with wrong file type") + { + std::shared_ptr wrong_file( + new storage_file("testContainerErrorHandling.root", 'o')); + CHECK_THROWS_AS(parent->set_file(wrong_file), std::runtime_error); + } } auto read_container = create_read_container(technology, "test/test_data"); From 31efd6717f7206d1130a1c5824920df618a7b6b5 Mon Sep 17 00:00:00 2001 From: Andrew Olivier Date: Thu, 10 Sep 2026 09:34:32 -0500 Subject: [PATCH 11/11] Addressed clang-tidy comment about putting a throw statement in braces. --- form/root_storage/root_rntuple_write_container.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/form/root_storage/root_rntuple_write_container.cpp b/form/root_storage/root_rntuple_write_container.cpp index 6f25d2d4b..f4921b91a 100644 --- a/form/root_storage/root_rntuple_write_container.cpp +++ b/form/root_storage/root_rntuple_write_container.cpp @@ -54,9 +54,10 @@ namespace form::detail::experimental { ROOT::RNTupleWriter& root_rntuple_write_container_imp::get_writer() { if (!writer_) { - if (!tfile_) + if (!tfile_) { throw std::runtime_error("root_rntuple_write_container_imp::setup_write no file loaded to " "write to on first fill() call"); + } writer_ = ROOT::RNTupleWriter::Append(std::move(model_), name(), *tfile_); }