From 95884d6235e4cecb8e8214683473987104d51f94 Mon Sep 17 00:00:00 2001 From: Chris Green Date: Fri, 3 Apr 2026 16:22:25 -0500 Subject: [PATCH 01/10] Resolve `bugprone-throwing-static-initialization` clang-tidy warnings --- .../root_tbranch_write_container.cpp | 27 +++++++++---------- phlex/core/detail/filter_impl.cpp | 12 ++++++--- phlex/model/data_layer_hierarchy.cpp | 7 +++-- test/data_cell_counting.cpp | 16 +++++++---- test/form/test_utils.hpp | 12 ++++----- 5 files changed, 43 insertions(+), 31 deletions(-) diff --git a/form/root_storage/root_tbranch_write_container.cpp b/form/root_storage/root_tbranch_write_container.cpp index 4a0132b00..3df526485 100644 --- a/form/root_storage/root_tbranch_write_container.cpp +++ b/form/root_storage/root_tbranch_write_container.cpp @@ -12,20 +12,6 @@ #include -namespace { - //Type name conversion based on https://root.cern.ch/doc/master/classTTree.html#ac1fa9466ce018d4aa739b357f981c615 - //An empty leaf list defaults to Float_t - std::unordered_map typeNameToLeafList = {{"int", "/I"}, - {"unsigned int", "/i"}, - {"float", "/F"}, - {"double", "/D"}, - {"short int", "/S"}, - {"unsigned short", "/s"}, - {"long int", "/L"}, - {"unsigned long int", "/l"}, - {"bool", "/O"}}; -} - using namespace form::detail::experimental; ROOT_TBranch_Write_ContainerImp::ROOT_TBranch_Write_ContainerImp(std::string const& name) : @@ -69,6 +55,19 @@ void ROOT_TBranch_Write_ContainerImp::setParent(std::shared_ptr typeNameToLeafList = { + {"int", "/I"}, + {"unsigned int", "/i"}, + {"float", "/F"}, + {"double", "/D"}, + {"short int", "/S"}, + {"unsigned short", "/s"}, + {"long int", "/L"}, + {"unsigned long int", "/l"}, + {"bool", "/O"}}; + if (m_tree == nullptr) { throw std::runtime_error("ROOT_TBranch_Write_ContainerImp::setupWrite no tree found"); } diff --git a/phlex/core/detail/filter_impl.cpp b/phlex/core/detail/filter_impl.cpp index 5a7cf673f..b84bdeea3 100644 --- a/phlex/core/detail/filter_impl.cpp +++ b/phlex/core/detail/filter_impl.cpp @@ -5,9 +5,13 @@ #include namespace { - phlex::product_query const output_dummy = phlex::product_query{ - .creator = "for_output_only"_id, .layer = "dummy_layer"_id, .suffix = "for_output_only"_id}; - phlex::product_queries const for_output_only{output_dummy}; + phlex::product_queries const for_output_only() + { + static phlex::product_query const output_dummy = phlex::product_query{ + .creator = "for_output_only"_id, .layer = "dummy_layer"_id, .suffix = "for_output_only"_id}; + static phlex::product_queries const for_output_only_queries{output_dummy}; + return for_output_only_queries; + } } namespace phlex::experimental { @@ -57,7 +61,7 @@ namespace phlex::experimental { assert(nargs_ > 0); } - data_map::data_map(for_output_t) : data_map{for_output_only} {} + data_map::data_map(for_output_t) : data_map{for_output_only()} {} void data_map::update(std::size_t const msg_id, product_store_const_ptr const& store) { diff --git a/phlex/model/data_layer_hierarchy.cpp b/phlex/model/data_layer_hierarchy.cpp index 1de22aad0..dc64972c8 100644 --- a/phlex/model/data_layer_hierarchy.cpp +++ b/phlex/model/data_layer_hierarchy.cpp @@ -6,8 +6,11 @@ #include "spdlog/spdlog.h" namespace { - std::string const unnamed{"(unnamed)"}; - std::string const& maybe_name(std::string const& name) { return empty(name) ? unnamed : name; } + std::string const& maybe_name(std::string const& name) + { + static constexpr std::string unnamed{"(unnamed)"}; + return empty(name) ? unnamed : name; + } } namespace phlex::experimental { diff --git a/test/data_cell_counting.cpp b/test/data_cell_counting.cpp index 0f0b90272..314e445c8 100644 --- a/test/data_cell_counting.cpp +++ b/test/data_cell_counting.cpp @@ -10,7 +10,13 @@ using namespace phlex::experimental::literals; using phlex::data_cell_index; namespace { - auto const job_hash_value = "job"_idq.hash; + struct job_hash_fixture { + std::size_t job_hash_value() + { + static std::size_t job_hash_value = "job"_idq.hash; + return job_hash_value; + } + }; } TEST_CASE("Counter with nothing processed", "[data model]") @@ -19,13 +25,13 @@ TEST_CASE("Counter with nothing processed", "[data model]") CHECK(job_counter.result().empty()); } -TEST_CASE("Counter one layer deep", "[data model]") +TEST_CASE_METHOD(job_hash_fixture, "Counter one layer deep", "[data model]") { data_cell_counter job_counter{}; for (std::size_t i = 0; i != 10; ++i) { job_counter.make_child("event"_id); } - auto const event_hash_value = hash(job_hash_value, "event"_idq.hash); + auto const event_hash_value = hash(job_hash_value(), "event"_idq.hash); CHECK(job_counter.result().count_for(event_hash_value) == 10); } @@ -56,7 +62,7 @@ TEST_CASE("Data layer hierarchy with ambiguous layer names", "[data model]") CHECK(h.count_for("/job/run/spill") == 2); } -TEST_CASE("Counter multiple layers deep", "[data model]") +TEST_CASE_METHOD(job_hash_fixture, "Counter multiple layers deep", "[data model]") { constexpr std::size_t nruns{2ull}; constexpr std::size_t nsubruns_per_run{3ull}; @@ -78,7 +84,7 @@ TEST_CASE("Counter multiple layers deep", "[data model]") CHECK(h.count_for("event", true) == processed_events); }; - auto const run_hash_value = hash(job_hash_value, "run"_idq.hash); + auto const run_hash_value = hash(job_hash_value(), "run"_idq.hash); auto const subrun_hash_value = hash(run_hash_value, "subrun"_idq.hash); auto const event_hash_value = hash(subrun_hash_value, "event"_idq.hash); diff --git a/test/form/test_utils.hpp b/test/form/test_utils.hpp index 99e51ee6a..6f9a2c78b 100644 --- a/test/form/test_utils.hpp +++ b/test/form/test_utils.hpp @@ -17,8 +17,8 @@ using namespace form::detail::experimental; namespace form::test { - inline std::string const testTreeName = "FORMTestTree"; - inline std::string const testFileName = "FORMTestFile.root"; + inline constexpr char const* testTreeName = "FORMTestTree"; + inline constexpr char const* testFileName = "FORMTestFile.root"; template inline std::string getTypeName() @@ -29,7 +29,7 @@ namespace form::test { template inline std::string makeTestBranchName() { - return testTreeName + "/" + getTypeName(); + return std::string(testTreeName) + "/" + getTypeName(); } inline std::vector> doWrite( @@ -66,8 +66,8 @@ namespace form::test { template inline void write(int const technology, PRODS&... prods) { - auto file = createFile(technology, testFileName.c_str(), 'o'); - auto parent = createWriteAssociation(technology, testTreeName); + auto file = createFile(technology, std::string(testFileName), 'o'); + auto parent = createWriteAssociation(technology, std::string(testTreeName)); parent->setFile(file); parent->setupWrite(); @@ -94,7 +94,7 @@ namespace form::test { template inline std::tuple...> read(int const technology) { - auto file = createFile(technology, testFileName, 'i'); + auto file = createFile(technology, std::string(testFileName), 'i'); return std::make_tuple(doRead(file, technology)...); } From 381db612e12211efa40a36e9f674de349bbd358e Mon Sep 17 00:00:00 2001 From: Chris Green Date: Tue, 7 Apr 2026 15:21:27 -0500 Subject: [PATCH 02/10] Update test/data_cell_counting.cpp Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- test/data_cell_counting.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/data_cell_counting.cpp b/test/data_cell_counting.cpp index 314e445c8..898913034 100644 --- a/test/data_cell_counting.cpp +++ b/test/data_cell_counting.cpp @@ -11,10 +11,10 @@ using phlex::data_cell_index; namespace { struct job_hash_fixture { - std::size_t job_hash_value() + std::size_t job_hash_value() const { - static std::size_t job_hash_value = "job"_idq.hash; - return job_hash_value; + static std::size_t cached_hash = "job"_idq.hash; + return cached_hash; } }; } From 606181aaa8cfc7bb2371219a2840cfd5f50af1b2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 7 Apr 2026 20:23:49 +0000 Subject: [PATCH 03/10] Return const& from for_output_only() to avoid dangling pointer Agent-Logs-Url: https://github.com/Framework-R-D/phlex/sessions/5501225a-2e96-449f-94e4-a1dc4a613a1c Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com> --- phlex/core/detail/filter_impl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phlex/core/detail/filter_impl.cpp b/phlex/core/detail/filter_impl.cpp index b84bdeea3..dfb11e899 100644 --- a/phlex/core/detail/filter_impl.cpp +++ b/phlex/core/detail/filter_impl.cpp @@ -5,7 +5,7 @@ #include namespace { - phlex::product_queries const for_output_only() + phlex::product_queries const& for_output_only() { static phlex::product_query const output_dummy = phlex::product_query{ .creator = "for_output_only"_id, .layer = "dummy_layer"_id, .suffix = "for_output_only"_id}; From a35ae09bdfbe981645b22b5f32116387c318dcc4 Mon Sep 17 00:00:00 2001 From: Chris Green Date: Tue, 7 Apr 2026 15:27:30 -0500 Subject: [PATCH 04/10] Prevent unwanted empty entries for missing fundamental type access attempts --- form/root_storage/root_tbranch_write_container.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/form/root_storage/root_tbranch_write_container.cpp b/form/root_storage/root_tbranch_write_container.cpp index 3df526485..8196b5a1a 100644 --- a/form/root_storage/root_tbranch_write_container.cpp +++ b/form/root_storage/root_tbranch_write_container.cpp @@ -57,7 +57,7 @@ void ROOT_TBranch_Write_ContainerImp::setupWrite(std::type_info const& type) { //Type name conversion based on https://root.cern.ch/doc/master/classTTree.html#ac1fa9466ce018d4aa739b357f981c615 //An empty leaf list defaults to Float_t - static std::unordered_map typeNameToLeafList = { + static std::unordered_map const typeNameToLeafList = { {"int", "/I"}, {"unsigned int", "/i"}, {"float", "/F"}, @@ -81,7 +81,7 @@ void ROOT_TBranch_Write_ContainerImp::setupWrite(std::type_info const& type) if (dictInfo->Property() & EProperty::kIsFundamental) { m_branch = m_tree->Branch(col_name().c_str(), static_cast(nullptr), - (col_name() + typeNameToLeafList[dictInfo->GetName()]).c_str(), + (col_name() + typeNameToLeafList.at(dictInfo->GetName())).c_str(), 4096); } else { m_branch = From e819f5a8cadfe75a1168318c29d80bed58d5fb94 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 7 Apr 2026 20:46:14 +0000 Subject: [PATCH 05/10] Revert typeNameToLeafList to non-const with [] access; empty leaf list is intentional Agent-Logs-Url: https://github.com/Framework-R-D/phlex/sessions/cb3139bb-776b-463b-8198-38267887c6a7 Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com> --- form/root_storage/root_tbranch_write_container.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/form/root_storage/root_tbranch_write_container.cpp b/form/root_storage/root_tbranch_write_container.cpp index 8196b5a1a..9d524bdef 100644 --- a/form/root_storage/root_tbranch_write_container.cpp +++ b/form/root_storage/root_tbranch_write_container.cpp @@ -56,8 +56,8 @@ void ROOT_TBranch_Write_ContainerImp::setParent(std::shared_ptr const typeNameToLeafList = { + //An empty leaf list (i.e. for a type not in this map) defaults to Float_t; this is intentional. + static std::unordered_map typeNameToLeafList = { {"int", "/I"}, {"unsigned int", "/i"}, {"float", "/F"}, @@ -81,7 +81,7 @@ void ROOT_TBranch_Write_ContainerImp::setupWrite(std::type_info const& type) if (dictInfo->Property() & EProperty::kIsFundamental) { m_branch = m_tree->Branch(col_name().c_str(), static_cast(nullptr), - (col_name() + typeNameToLeafList.at(dictInfo->GetName())).c_str(), + (col_name() + typeNameToLeafList[dictInfo->GetName()]).c_str(), 4096); } else { m_branch = From 76d7ec6d4fba991a42066a5c6a1c0fc770a59860 Mon Sep 17 00:00:00 2001 From: Chris Green Date: Tue, 7 Apr 2026 17:22:54 -0500 Subject: [PATCH 06/10] `cached_hash` should be `const` - Per https://github.com/Framework-R-D/phlex/pull/472#discussion_r3047903578 --- test/data_cell_counting.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/data_cell_counting.cpp b/test/data_cell_counting.cpp index 898913034..08f19ebf9 100644 --- a/test/data_cell_counting.cpp +++ b/test/data_cell_counting.cpp @@ -13,7 +13,7 @@ namespace { struct job_hash_fixture { std::size_t job_hash_value() const { - static std::size_t cached_hash = "job"_idq.hash; + static std::size_t const cached_hash = "job"_idq.hash; return cached_hash; } }; From efc7d1cbbf6235f0986af8e5c524a7f108da7895 Mon Sep 17 00:00:00 2001 From: Chris Green Date: Tue, 7 Apr 2026 17:34:11 -0500 Subject: [PATCH 07/10] Report on `clang-tidy` fix progress --- docs/dev/clang-tidy-fixes-2026-04.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/dev/clang-tidy-fixes-2026-04.md b/docs/dev/clang-tidy-fixes-2026-04.md index 39358f829..b4e41de72 100644 --- a/docs/dev/clang-tidy-fixes-2026-04.md +++ b/docs/dev/clang-tidy-fixes-2026-04.md @@ -11,5 +11,5 @@ - [x] [bugprone-random-generator-seed](https://github.com/Framework-R-D/phlex/pull/490) (1) - [x] [bugprone-reserved-identifier](https://github.com/Framework-R-D/phlex/pull/489) (1) - [ ] bugprone-return-const-ref-from-parameter (163) - - [ ] bugprone-throwing-static-initialization (11) + - [x] [bugprone-throwing-static-initialization](https://github.com/Framework-R-D/phlex/pull/472) (11) - [ ] bugprone-unchecked-optional-access (15) From 8562839a620024acf9da594deccc77a8b1e7b2e4 Mon Sep 17 00:00:00 2001 From: Chris Green Date: Tue, 7 Apr 2026 17:38:54 -0500 Subject: [PATCH 08/10] `constexpr` -> `const` - Per https://github.com/Framework-R-D/phlex/pull/472#discussion_r3047891788 --- phlex/model/data_layer_hierarchy.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phlex/model/data_layer_hierarchy.cpp b/phlex/model/data_layer_hierarchy.cpp index dc64972c8..8a2e15a68 100644 --- a/phlex/model/data_layer_hierarchy.cpp +++ b/phlex/model/data_layer_hierarchy.cpp @@ -8,7 +8,7 @@ namespace { std::string const& maybe_name(std::string const& name) { - static constexpr std::string unnamed{"(unnamed)"}; + static std::string const unnamed{"(unnamed)"}; return empty(name) ? unnamed : name; } } From 6898384f4c52e819d91b2ec4486ce61cb5be6c50 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 7 Apr 2026 23:22:15 +0000 Subject: [PATCH 09/10] Add tests for filter_impl.cpp and data_layer_hierarchy.cpp coverage gaps Agent-Logs-Url: https://github.com/Framework-R-D/phlex/sessions/d1188b31-3b1a-484b-8300-701ba55a692a Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com> --- test/data_cell_counting.cpp | 11 +++++++++++ test/filter_impl.cpp | 18 ++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/test/data_cell_counting.cpp b/test/data_cell_counting.cpp index 08f19ebf9..ced74dab8 100644 --- a/test/data_cell_counting.cpp +++ b/test/data_cell_counting.cpp @@ -62,6 +62,17 @@ TEST_CASE("Data layer hierarchy with ambiguous layer names", "[data model]") CHECK(h.count_for("/job/run/spill") == 2); } +TEST_CASE("Data layer hierarchy with unnamed layer", "[data model]") +{ + // Exercises the maybe_name() fallback to "(unnamed)" for layers with empty names. + // graph_layout() is called from the destructor, which traverses the hierarchy + // and invokes maybe_name("") for the unnamed child. + data_layer_hierarchy h; + auto job_index = data_cell_index::job(); + h.increment_count(job_index); + h.increment_count(job_index->make_child("", 0)); +} + TEST_CASE_METHOD(job_hash_fixture, "Counter multiple layers deep", "[data model]") { constexpr std::size_t nruns{2ull}; diff --git a/test/filter_impl.cpp b/test/filter_impl.cpp index 4504ff4d7..1197e8714 100644 --- a/test/filter_impl.cpp +++ b/test/filter_impl.cpp @@ -66,3 +66,21 @@ TEST_CASE("Filter data map", "[filtering]") data.update(msg_id, store_with_b); CHECK(data.is_complete(msg_id)); } + +TEST_CASE("Data map for output only", "[filtering]") +{ + // Exercises data_map(for_output_t) which uses the output-only product query + data_map data{data_map::for_output}; + + std::size_t const msg_id{1}; + CHECK(not data.is_complete(msg_id)); + + // Output-only data maps accept any store (no product lookup required) + auto store = product_store::base(algorithm_name::create("output_only")); + data.update(msg_id, store); + CHECK(data.is_complete(msg_id)); + + auto result = data.release_data(msg_id); + CHECK(result.size() == 1); + CHECK(not data.is_complete(msg_id)); +} From 7b48c74c094d28bdf618eeaddc1c133d400fa774 Mon Sep 17 00:00:00 2001 From: Chris Green Date: Wed, 8 Apr 2026 08:27:15 -0500 Subject: [PATCH 10/10] Update test/data_cell_counting.cpp Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- test/data_cell_counting.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/data_cell_counting.cpp b/test/data_cell_counting.cpp index ced74dab8..2548557e0 100644 --- a/test/data_cell_counting.cpp +++ b/test/data_cell_counting.cpp @@ -65,12 +65,13 @@ TEST_CASE("Data layer hierarchy with ambiguous layer names", "[data model]") TEST_CASE("Data layer hierarchy with unnamed layer", "[data model]") { // Exercises the maybe_name() fallback to "(unnamed)" for layers with empty names. - // graph_layout() is called from the destructor, which traverses the hierarchy - // and invokes maybe_name("") for the unnamed child. + // Invoke print() explicitly so the test body, rather than teardown side effects, + // traverses the hierarchy and reaches maybe_name("") for the unnamed child. data_layer_hierarchy h; auto job_index = data_cell_index::job(); - h.increment_count(job_index); - h.increment_count(job_index->make_child("", 0)); + CHECK_NOTHROW(h.increment_count(job_index)); + CHECK_NOTHROW(h.increment_count(job_index->make_child("", 0))); + CHECK_NOTHROW(h.print()); } TEST_CASE_METHOD(job_hash_fixture, "Counter multiple layers deep", "[data model]")