diff --git a/form/root_storage/root_rfield_write_container.cpp b/form/root_storage/root_rfield_write_container.cpp index 3d814296e..c058ab25a 100644 --- a/form/root_storage/root_rfield_write_container.cpp +++ b/form/root_storage/root_rfield_write_container.cpp @@ -30,21 +30,13 @@ 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_) { + //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 failed to get a TFile from a root_tfile_imp"); + "root_rfield_write_container_imp::set_file was passed a file that is not a ROOT file."); } + + storage_write_container::set_file(file); } void root_rfield_write_container_imp::set_parent( @@ -67,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() @@ -92,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. @@ -132,7 +115,7 @@ namespace form::detail::experimental { } } - rntuple_parent_->model->AddField(std::move(field)); + assert(rntuple_parent_->get_model()); + 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 5206ef1bb..1281bbdfa 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 60de09df2..6f25d2d4b 100644 --- a/form/root_storage/root_rntuple_write_container.cpp +++ b/form/root_storage/root_rntuple_write_container.cpp @@ -12,20 +12,33 @@ 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_) { + try { + writer_->CommitDataset(); + } catch (ROOT::RException const& e) { + std::cerr << "Failed to commit RNTuple " << name() << " at destruction.\n"; + } } } 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."); + } } std::uint64_t root_rntuple_write_container_imp::fill(void const* /*data*/) @@ -38,5 +51,30 @@ namespace form::detail::experimental { throw std::runtime_error("root_rntuple_write_container_imp::commit not implemented"); } + ROOT::RNTupleWriter& root_rntuple_write_container_imp::get_writer() + { + 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_); + } + + return *writer_; + } + + std::unique_ptr const& root_rntuple_write_container_imp::get_model() const + { + return model_; + } + + RRawPtrWriteEntry& root_rntuple_write_container_imp::get_entry() + { + if (!entry_) { + entry_ = get_writer().CreateRawPtrWriteEntry(); + } + return *entry_; + } + void root_rntuple_write_container_imp::setup_write(std::type_info const& /*type*/) {} } diff --git a/form/root_storage/root_rntuple_write_container.hpp b/form/root_storage/root_rntuple_write_container.hpp index 9de6c322e..a5a018f69 100644 --- a/form/root_storage/root_rntuple_write_container.hpp +++ b/form/root_storage/root_rntuple_write_container.hpp @@ -57,10 +57,21 @@ namespace form::detail::experimental { 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() const; + 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; + std::unique_ptr writer_; + std::unique_ptr model_; + std::unique_ptr entry_; }; } 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");