From 3fca8a3eeef46e4e5e7574eef7eb53f50ecc9ab9 Mon Sep 17 00:00:00 2001 From: wwuoneway Date: Thu, 29 Oct 2026 15:19:09 -0500 Subject: [PATCH 1/2] form: widen the read path to 64-bit rows --- form/persistence/persistence_reader.cpp | 3 ++- .../root_storage/root_rfield_read_container.cpp | 10 ++++++---- .../root_storage/root_rfield_read_container.hpp | 5 +++-- .../root_tbranch_read_container.cpp | 14 +++++++++----- .../root_tbranch_read_container.hpp | 5 +++-- form/storage/istorage.hpp | 11 ++++++----- form/storage/storage_read_container.cpp | 5 +++-- form/storage/storage_read_container.hpp | 5 +++-- form/storage/storage_reader.cpp | 17 +++++++++-------- form/storage/storage_reader.hpp | 12 +++++++----- test/form/form_multiple_outputs_check.cpp | 2 +- test/form/navigation_check.hpp | 4 ++-- 12 files changed, 54 insertions(+), 39 deletions(-) diff --git a/form/persistence/persistence_reader.cpp b/form/persistence/persistence_reader.cpp index 58f507171..c0ea8fbfb 100644 --- a/form/persistence/persistence_reader.cpp +++ b/form/persistence/persistence_reader.cpp @@ -8,6 +8,7 @@ #include "persistence_utils.hpp" #include "storage/istorage.hpp" +#include #include #include #include @@ -104,7 +105,7 @@ std::unique_ptr persistence_reader::get_token(std::string const& creator, std::string const full_label = build_full_label(row_space, label); std::string const index_label = build_full_label(row_space, "index"); - int const row_id = store_reader_->get_index( + std::uint64_t const row_id = store_reader_->get_index( token{config_item->file_name, index_label, config_item->technology}, id, tech_settings_); return std::make_unique( config_item->file_name, full_label, config_item->technology, row_id); diff --git a/form/root_storage/root_rfield_read_container.cpp b/form/root_storage/root_rfield_read_container.cpp index 28095363b..34250d69c 100644 --- a/form/root_storage/root_rfield_read_container.cpp +++ b/form/root_storage/root_rfield_read_container.cpp @@ -17,13 +17,13 @@ #include // IWYU pragma: keep #include +#include #include #include #include #include #include #include -#include namespace { std::mutex& root_rfield_read_mutex() @@ -87,7 +87,9 @@ namespace form::detail::experimental { } } - bool root_rfield_read_container_imp::read(int id, void const** data, std::type_info const& type) + bool root_rfield_read_container_imp::read(std::uint64_t id, + void const** data, + std::type_info const& type) { std::scoped_lock const guard(root_rfield_read_mutex()); try { @@ -117,7 +119,7 @@ namespace form::detail::experimental { return true; } - int root_rfield_read_container_imp::entries() + std::uint64_t root_rfield_read_container_imp::entries() { std::scoped_lock const guard(root_rfield_read_mutex()); @@ -140,7 +142,7 @@ namespace form::detail::experimental { " does not exist"); } - return static_cast(reader_->GetNEntries()); + return static_cast(reader_->GetNEntries()); } void root_rfield_read_container_imp::create_view(std::type_info const& type) diff --git a/form/root_storage/root_rfield_read_container.hpp b/form/root_storage/root_rfield_read_container.hpp index cf98460d7..724175970 100644 --- a/form/root_storage/root_rfield_read_container.hpp +++ b/form/root_storage/root_rfield_read_container.hpp @@ -5,6 +5,7 @@ #include "storage/storage_read_container.hpp" +#include #include #include @@ -36,8 +37,8 @@ namespace form::detail::experimental { void set_file(std::shared_ptr file) override; void prime(std::type_info const& type) override; - bool read(int id, void const** data, std::type_info const& type) override; - int entries() override; + bool read(std::uint64_t id, void const** data, std::type_info const& type) override; + std::uint64_t entries() override; private: std::shared_ptr tfile_; diff --git a/form/root_storage/root_tbranch_read_container.cpp b/form/root_storage/root_tbranch_read_container.cpp index 01ac8dd6e..b9ee8bcba 100644 --- a/form/root_storage/root_tbranch_read_container.cpp +++ b/form/root_storage/root_tbranch_read_container.cpp @@ -17,11 +17,13 @@ #include #include +#include #include #include #include #include #include +#include using namespace form::detail::experimental; @@ -86,7 +88,9 @@ void root_tbranch_read_container_imp::prime(std::type_info const& type) } } -bool root_tbranch_read_container_imp::read(int id, void const** data, std::type_info const& type) +bool root_tbranch_read_container_imp::read(std::uint64_t id, + void const** data, + std::type_info const& type) { std::scoped_lock const guard(root_tbranch_read_mutex()); @@ -106,7 +110,7 @@ bool root_tbranch_read_container_imp::read(int id, void const** data, std::type_ if (branch_ == nullptr) { throw std::runtime_error("root_tbranch_read_container_imp::read no branch found"); } - if (id >= tree_->GetEntries()) { + if (std::cmp_greater_equal(id, tree_->GetEntries())) { return false; } @@ -194,7 +198,7 @@ bool root_tbranch_read_container_imp::read(int id, void const** data, std::type_ std::to_string(branch_status)); } - Long64_t const tentry = tree_->LoadTree(id); + Long64_t const tentry = tree_->LoadTree(static_cast(id)); branch_->GetEntry(tentry); *data = branch_buffer; @@ -204,7 +208,7 @@ bool root_tbranch_read_container_imp::read(int id, void const** data, std::type_ return true; } -int root_tbranch_read_container_imp::entries() +std::uint64_t root_tbranch_read_container_imp::entries() { std::scoped_lock const guard(root_tbranch_read_mutex()); @@ -224,5 +228,5 @@ int root_tbranch_read_container_imp::entries() if (branch_ == nullptr) { throw std::runtime_error("root_tbranch_read_container_imp::entries no branch found"); } - return static_cast(tree_->GetEntries()); + return static_cast(tree_->GetEntries()); } diff --git a/form/root_storage/root_tbranch_read_container.hpp b/form/root_storage/root_tbranch_read_container.hpp index 32c518c2d..28d803f72 100644 --- a/form/root_storage/root_tbranch_read_container.hpp +++ b/form/root_storage/root_tbranch_read_container.hpp @@ -5,6 +5,7 @@ #include "storage/storage_read_container.hpp" +#include #include #include @@ -25,8 +26,8 @@ namespace form::detail::experimental { void set_file(std::shared_ptr file) override; void prime(std::type_info const& type) override; - bool read(int id, void const** data, std::type_info const& type) override; - int entries() override; + bool read(std::uint64_t id, void const** data, std::type_info const& type) override; + std::uint64_t entries() override; private: std::shared_ptr tfile_; diff --git a/form/storage/istorage.hpp b/form/storage/istorage.hpp index 93f22ff1a..1a935a110 100644 --- a/form/storage/istorage.hpp +++ b/form/storage/istorage.hpp @@ -25,9 +25,10 @@ namespace form::detail::experimental { i_storage_reader() = default; virtual ~i_storage_reader() = default; - virtual int get_index(token const& token, - std::string const& id, - form::experimental::config::tech_setting_config const& settings) = 0; + virtual std::uint64_t get_index( + token const& token, + std::string const& id, + form::experimental::config::tech_setting_config const& settings) = 0; virtual void prime(token const& token, std::type_info const& type, form::experimental::config::tech_setting_config const& settings) = 0; @@ -99,8 +100,8 @@ namespace form::detail::experimental { virtual void set_file(std::shared_ptr file) = 0; virtual void prime(std::type_info const& type) = 0; - virtual bool read(int id, void const** data, std::type_info const& type) = 0; - virtual int entries() = 0; + virtual bool read(std::uint64_t id, void const** data, std::type_info const& type) = 0; + virtual std::uint64_t entries() = 0; virtual void set_attribute(std::string const& name, std::string const& value) = 0; }; diff --git a/form/storage/storage_read_container.cpp b/form/storage/storage_read_container.cpp index 7aee796fc..7586255ea 100644 --- a/form/storage/storage_read_container.cpp +++ b/form/storage/storage_read_container.cpp @@ -5,6 +5,7 @@ #include "core/container_naming.hpp" #include "storage/istorage.hpp" +#include #include #include #include @@ -27,14 +28,14 @@ void storage_read_container::set_file(std::shared_ptr file) { fi void storage_read_container::prime(std::type_info const& /*type*/) {} -bool storage_read_container::read(int /* id*/, +bool storage_read_container::read(std::uint64_t /* id*/, void const** /*data*/, std::type_info const& /* type*/) { return false; } -int storage_read_container::entries() { return 0; } +std::uint64_t storage_read_container::entries() { return 0; } void storage_read_container::set_attribute(std::string const& /*name*/, std::string const& /*value*/) diff --git a/form/storage/storage_read_container.hpp b/form/storage/storage_read_container.hpp index dbe33fcbd..cd43af6c5 100644 --- a/form/storage/storage_read_container.hpp +++ b/form/storage/storage_read_container.hpp @@ -5,6 +5,7 @@ #include "istorage.hpp" +#include #include #include @@ -24,8 +25,8 @@ namespace form::detail::experimental { void set_file(std::shared_ptr file) override; void prime(std::type_info const& type) override; - bool read(int id, void const** data, std::type_info const& type) override; - int entries() override; + bool read(std::uint64_t id, void const** data, std::type_info const& type) override; + std::uint64_t entries() override; void set_attribute(std::string const& name, std::string const& value) override; diff --git a/form/storage/storage_reader.cpp b/form/storage/storage_reader.cpp index ed8d5b116..42fb42b5e 100644 --- a/form/storage/storage_reader.cpp +++ b/form/storage/storage_reader.cpp @@ -9,6 +9,7 @@ #include "storage/istorage.hpp" #include +#include #include #include #include @@ -60,9 +61,10 @@ namespace form::detail::experimental { } } -int storage_reader::get_index(token const& token, - std::string const& id, - form::experimental::config::tech_setting_config const& settings) +std::uint64_t storage_reader::get_index( + token const& token, + std::string const& id, + form::experimental::config::tech_setting_config const& settings) { auto const& index = index_map(token, settings); auto const found = index.find(id); @@ -113,7 +115,7 @@ std::vector storage_reader::list_indices( return result; } -std::map const& storage_reader::index_map( +std::map const& storage_reader::index_map( token const& token, form::experimental::config::tech_setting_config const& settings) { auto const cont_key = std::make_pair(token.file_name(), token.container_name()); @@ -143,9 +145,9 @@ std::map const& storage_reader::index_map( cont->second->set_file(file->second); } - std::map index; + std::map index; auto const& type = typeid(std::string); - int row = 0; + std::uint64_t row = 0; void const* raw_data = nullptr; while (cont->second->read(row, &raw_data, type)) { std::unique_ptr const id(static_cast(raw_data)); @@ -189,6 +191,5 @@ void storage_reader::read_container(token const& token, cont->second->set_attribute(key, value); } } - // TODO: token::id() is a 64-bit row; the read container interface still takes an int entry. Narrow explicitly here (exact for all realistic row counts). Widening the read path to 64-bit is a follow-up PR. - cont->second->read(static_cast(token.id()), data, type); + cont->second->read(token.id(), data, type); } diff --git a/form/storage/storage_reader.hpp b/form/storage/storage_reader.hpp index dea2f054f..3a8a1128a 100644 --- a/form/storage/storage_reader.hpp +++ b/form/storage/storage_reader.hpp @@ -6,6 +6,7 @@ #include "istorage.hpp" #include "storage_utils.hpp" +#include #include #include #include @@ -19,9 +20,10 @@ namespace form::detail::experimental { storage_reader() = default; ~storage_reader() override = default; - int get_index(token const& token, - std::string const& id, - form::experimental::config::tech_setting_config const& settings) override; + std::uint64_t get_index( + token const& token, + std::string const& id, + form::experimental::config::tech_setting_config const& settings) override; void prime(token const& token, std::type_info const& type, form::experimental::config::tech_setting_config const& settings) override; @@ -33,7 +35,7 @@ namespace form::detail::experimental { form::experimental::config::tech_setting_config const& settings) override; private: - std::map const& index_map( + std::map const& index_map( token const& token, form::experimental::config::tech_setting_config const& settings); std::map> files_; @@ -41,7 +43,7 @@ namespace form::detail::experimental { std::shared_ptr, pair_hash> read_containers_; - std::map, std::map> index_maps_; + std::map, std::map> index_maps_; }; } // namespace form::detail::experimental diff --git a/test/form/form_multiple_outputs_check.cpp b/test/form/form_multiple_outputs_check.cpp index 6e4c16af4..d47814632 100644 --- a/test/form/form_multiple_outputs_check.cpp +++ b/test/form/form_multiple_outputs_check.cpp @@ -95,7 +95,7 @@ namespace { checks.check(false, std::format("{} has a parsable data cell {}", what, id)); continue; } - auto const row = static_cast(storage.get_index(index, id, settings)); + auto const row = storage.get_index(index, id, settings); token const location{ file_name, build_full_label(row_space, product.product_name), technology, row}; checks.check(matches_example(storage, location, product.product_name, *layers), diff --git a/test/form/navigation_check.hpp b/test/form/navigation_check.hpp index 6357580e1..1a67bfdfa 100644 --- a/test/form/navigation_check.hpp +++ b/test/form/navigation_check.hpp @@ -117,7 +117,7 @@ namespace form::test { { } - std::optional rows(std::string const& container) + std::optional rows(std::string const& container) { try { return container_for(container)->entries(); @@ -134,7 +134,7 @@ namespace form::test { std::vector values; try { auto const cont = container_for(container); - for (int row = 0, total = cont->entries(); row < total; ++row) { + for (std::uint64_t row = 0, total = cont->entries(); row < total; ++row) { void const* raw = nullptr; if (!cont->read(row, &raw, typeid(T))) { break; From 4cae24d66ada8ab4dfe9801c253de3c440804ed2 Mon Sep 17 00:00:00 2001 From: wwuoneway Date: Thu, 29 Oct 2026 17:06:10 -0500 Subject: [PATCH 2/2] form: pass LoadTree() result directly to GetEntry(), keep Long64_t only at the ROOT call; FORM's row stays std::uint64_t. test(form): compare 64-bit rows without int casts: get_index() now returns std::uint64_t; drop the int casts from the merged #992 tests, which fail GCC's -Werror=sign-compare. Split fundamental buffer allocation out of TBranch read(), Fixes readability-function-size on read(), whose signature this PR changes. No behavior change. --- .../root_tbranch_read_container.cpp | 87 +++++++++---------- test/form/form_storage_test.cpp | 18 ++-- 2 files changed, 47 insertions(+), 58 deletions(-) diff --git a/form/root_storage/root_tbranch_read_container.cpp b/form/root_storage/root_tbranch_read_container.cpp index b9ee8bcba..043303d64 100644 --- a/form/root_storage/root_tbranch_read_container.cpp +++ b/form/root_storage/root_tbranch_read_container.cpp @@ -33,6 +33,43 @@ namespace { static std::mutex m; return m; } + + // Allocates a buffer for a fundamental branch type; the caller owns it. + gsl::owner new_fundamental(Int_t data_type, std::type_info const& type) + { + switch (data_type) { + case kChar_t: + return new Char_t; + case kUChar_t: + return new UChar_t; + case kShort_t: + return new Short_t; + case kUShort_t: + return new UShort_t; + case kInt_t: + return new Int_t; + case kUInt_t: + return new UInt_t; + case kLong_t: + return new Long_t; + case kULong_t: + return new ULong_t; + case kLong64_t: + return new Long64_t; + case kULong64_t: + return new ULong64_t; + case kFloat_t: + return new Float_t; + case kDouble_t: + return new Double_t; + case kBool_t: + return new Bool_t; + default: + throw std::runtime_error( + std::string{"root_tbranch_read_container_imp::read unsupported fundamental type: "} + + demangle_name(type)); + } + } } root_tbranch_read_container_imp::root_tbranch_read_container_imp(std::string const& name) : @@ -127,51 +164,7 @@ bool root_tbranch_read_container_imp::read(std::uint64_t id, if (dict_info->Property() & EProperty::kIsFundamental) { // NOLINTNEXTLINE(cppcoreguidelines-pro-type-static-cast-downcast) auto* fund_info = static_cast(dict_info); // Already checked to be fundamental - switch (fund_info->GetType()) { - case kChar_t: - branch_buffer = new Char_t; - break; - case kUChar_t: - branch_buffer = new UChar_t; - break; - case kShort_t: - branch_buffer = new Short_t; - break; - case kUShort_t: - branch_buffer = new UShort_t; - break; - case kInt_t: - branch_buffer = new Int_t; - break; - case kUInt_t: - branch_buffer = new UInt_t; - break; - case kLong_t: - branch_buffer = new Long_t; - break; - case kULong_t: - branch_buffer = new ULong_t; - break; - case kLong64_t: - branch_buffer = new Long64_t; - break; - case kULong64_t: - branch_buffer = new ULong64_t; - break; - case kFloat_t: - branch_buffer = new Float_t; - break; - case kDouble_t: - branch_buffer = new Double_t; - break; - case kBool_t: - branch_buffer = new Bool_t; - break; - default: - throw std::runtime_error( - std::string{"root_tbranch_read_container_imp::read unsupported fundamental type: "} + - demangle_name(type)); - }; + branch_buffer = new_fundamental(fund_info->GetType(), type); branch_status = tree_->SetBranchAddress( col_name().c_str(), branch_buffer, nullptr, EDataType(fund_info->GetType()), false); } else { @@ -198,8 +191,8 @@ bool root_tbranch_read_container_imp::read(std::uint64_t id, std::to_string(branch_status)); } - Long64_t const tentry = tree_->LoadTree(static_cast(id)); - branch_->GetEntry(tentry); + // ROOT addresses entries as Long64_t; id < GetEntries() was checked above. + branch_->GetEntry(tree_->LoadTree(static_cast(id))); *data = branch_buffer; // Reset the branch address to avoid unwanted ownership issues. diff --git a/test/form/form_storage_test.cpp b/test/form/form_storage_test.cpp index cf9f0a9a3..7480982c9 100644 --- a/test/form/form_storage_test.cpp +++ b/test/form/form_storage_test.cpp @@ -147,13 +147,11 @@ namespace { form::experimental::config::tech_setting_config const settings{}; auto const row = reader.get_index(index_token_of(file_name, creator), id, settings); void const* raw = nullptr; - reader.read_container(token{file_name, - build_full_label(row_space(creator), "prod"), - technology, - static_cast(row)}, - &raw, - typeid(std::vector), - settings); + reader.read_container( + token{file_name, build_full_label(row_space(creator), "prod"), technology, row}, + &raw, + typeid(std::vector), + settings); std::unique_ptr const> const payload{ static_cast const*>(raw)}; REQUIRE(payload != nullptr); @@ -621,8 +619,7 @@ TEST_CASE("storage_reader get_index: cells written out of order resolve to their form::experimental::config::tech_setting_config const settings{}; CHECK(reader.list_indices(index_token_of(file_name, creator), settings) == ids_of(cells)); for (std::size_t row = 0; row != cells.size(); ++row) { - CHECK(reader.get_index(index_token_of(file_name, creator), cells[row].id, settings) == - static_cast(row)); + CHECK(reader.get_index(index_token_of(file_name, creator), cells[row].id, settings) == row); CHECK(read_cell(reader, file_name, creator, cells[row].id) == payload_of(cells[row])); } }; @@ -670,8 +667,7 @@ TEST_CASE("storage_reader get_index: index maps are kept per file", "[form]") for (std::size_t row = 0; row != first_cells.size(); ++row) { for (auto const& [file_name, cells] : {std::pair{first_file, first_cells}, std::pair{second_file, second_cells}}) { - CHECK(reader.get_index(index_token_of(file_name, creator), cells[row].id, settings) == - static_cast(row)); + CHECK(reader.get_index(index_token_of(file_name, creator), cells[row].id, settings) == row); CHECK(read_cell(reader, file_name, creator, cells[row].id) == payload_of(cells[row])); } }