From 0792983f505cacc08a9e13b988faee0209aecefa Mon Sep 17 00:00:00 2001 From: Kyle Knoepfel Date: Fri, 21 Aug 2026 08:39:15 -0500 Subject: [PATCH] fix: resolve dangling clang-tidy warnings in core and tests --- docs/dev/clang-tidy-fixes-2026-04.md | 3 ++- form/root_storage/root_rfield_read_container.cpp | 15 +++++++-------- form/root_storage/root_rfield_write_container.cpp | 4 +--- form/root_storage/root_rfield_write_container.hpp | 2 +- .../root_storage/root_rntuple_write_container.cpp | 3 +-- .../root_storage/root_rntuple_write_container.hpp | 6 ++---- phlex/core/index_router.cpp | 9 +++------ plugins/layer_generator.cpp | 5 ++++- test/demo-giantdata/waveform_generator.cpp | 2 -- test/demo-giantdata/waveform_generator.hpp | 2 +- test/flush_gate_test.cpp | 4 ++-- 11 files changed, 24 insertions(+), 31 deletions(-) diff --git a/docs/dev/clang-tidy-fixes-2026-04.md b/docs/dev/clang-tidy-fixes-2026-04.md index 85d9194da..0264b211f 100644 --- a/docs/dev/clang-tidy-fixes-2026-04.md +++ b/docs/dev/clang-tidy-fixes-2026-04.md @@ -22,7 +22,8 @@ - [x] [bugprone-throwing-static-initialization](https://clang.llvm.org/extra/clang-tidy/checks/bugprone/throwing-static-initialization.html) (7) - [PR #472](https://github.com/Framework-R-D/phlex/pull/472) - [ ] [bugprone-branch-clone](https://clang.llvm.org/extra/clang-tidy/checks/bugprone/branch-clone.html) (12) -- [ ] [bugprone-throw-keyword-missing](https://clang.llvm.org/extra/clang-tidy/checks/bugprone/throw-keyword-missing.html) (6) +- [x] [bugprone-throw-keyword-missing](https://clang.llvm.org/extra/clang-tidy/checks/bugprone/throw-keyword-missing.html) (6) + - [PR #836](https://github.com/Framework-R-D/phlex/pull/836) - [x] [bugprone-unchecked-optional-access](https://clang.llvm.org/extra/clang-tidy/checks/bugprone/unchecked-optional-access.html) (15) - [PR #495](https://github.com/Framework-R-D/phlex/pull/495) - [x] [cert-dcl50-cpp](https://clang.llvm.org/extra/clang-tidy/checks/cert/dcl50-cpp.html) (2) diff --git a/form/root_storage/root_rfield_read_container.cpp b/form/root_storage/root_rfield_read_container.cpp index 72f84f797..19ba10a86 100644 --- a/form/root_storage/root_rfield_read_container.cpp +++ b/form/root_storage/root_rfield_read_container.cpp @@ -11,6 +11,7 @@ #include #include +#include namespace { std::mutex& root_rfield_read_mutex() @@ -26,13 +27,13 @@ namespace form::detail::experimental { { } - ROOT_RField_Read_ContainerImp::~ROOT_RField_Read_ContainerImp() {} + ROOT_RField_Read_ContainerImp::~ROOT_RField_Read_ContainerImp() = default; void ROOT_RField_Read_ContainerImp::setFile(std::shared_ptr file) { Storage_Read_Container::setFile(file); - auto form_root_file = dynamic_cast(file.get()); + auto* form_root_file = dynamic_cast(file.get()); if (form_root_file) { m_tfile = form_root_file->getTFile(); } else { @@ -45,13 +46,11 @@ namespace form::detail::experimental { throw std::runtime_error( "ROOT_RField_Read_ContainerImp::setFile failed to get a TFile from a ROOT_TFileImp"); } - - return; } void ROOT_RField_Read_ContainerImp::prime(std::type_info const& type) { - std::lock_guard guard(root_rfield_read_mutex()); + std::scoped_lock guard(root_rfield_read_mutex()); if (!m_tfile) { throw std::runtime_error("ROOT_RField_Read_ContainerImp::prime No file loaded"); @@ -72,14 +71,14 @@ namespace form::detail::experimental { bool ROOT_RField_Read_ContainerImp::read(int id, void const** data, std::type_info const& type) { - std::lock_guard guard(root_rfield_read_mutex()); + std::scoped_lock guard(root_rfield_read_mutex()); //Connect to file at the last possible moment at the cost of a little run-time branching if (!m_view) { createView(type); } - if (id >= static_cast(m_reader->GetNEntries())) { + if (std::cmp_greater_equal(id, m_reader->GetNEntries())) { return false; } @@ -103,7 +102,7 @@ namespace form::detail::experimental { int ROOT_RField_Read_ContainerImp::entries() { - std::lock_guard guard(root_rfield_read_mutex()); + std::scoped_lock guard(root_rfield_read_mutex()); if (!m_reader) { if (!m_tfile) { diff --git a/form/root_storage/root_rfield_write_container.cpp b/form/root_storage/root_rfield_write_container.cpp index 6a7b753df..cbc8d55bc 100644 --- a/form/root_storage/root_rfield_write_container.cpp +++ b/form/root_storage/root_rfield_write_container.cpp @@ -45,8 +45,6 @@ namespace form::detail::experimental { throw std::runtime_error( "ROOT_RField_Write_ContainerImp::setFile failed to get a TFile from a ROOT_TFileImp"); } - - return; } void ROOT_RField_Write_ContainerImp::setParent(std::shared_ptr parent) @@ -127,7 +125,7 @@ namespace form::detail::experimental { << type_name << ". This class is probably using something obsolete like TLorentzVector. Storing it " "in streamer mode to keep the application going." - << std::endl; + << '\n'; field = std::make_unique(col_name(), type_name); } } diff --git a/form/root_storage/root_rfield_write_container.hpp b/form/root_storage/root_rfield_write_container.hpp index 833086b87..6aa89ab20 100644 --- a/form/root_storage/root_rfield_write_container.hpp +++ b/form/root_storage/root_rfield_write_container.hpp @@ -22,7 +22,7 @@ namespace form::detail::experimental { void setFile(std::shared_ptr file) override; void setupWrite(std::type_info const& type) override; - void setParent(std::shared_ptr const parent) override; + void setParent(std::shared_ptr parent) override; std::uint64_t fill(void const* data) override; void commit() override; diff --git a/form/root_storage/root_rntuple_write_container.cpp b/form/root_storage/root_rntuple_write_container.cpp index e3fb4bbe5..10b551ff4 100644 --- a/form/root_storage/root_rntuple_write_container.cpp +++ b/form/root_storage/root_rntuple_write_container.cpp @@ -26,7 +26,6 @@ namespace form::detail::experimental { void ROOT_RNTuple_Write_ContainerImp::setFile(std::shared_ptr file) { Storage_Write_Container::setFile(file); - return; } std::uint64_t ROOT_RNTuple_Write_ContainerImp::fill(void const* /*data*/) @@ -39,5 +38,5 @@ namespace form::detail::experimental { throw std::runtime_error("ROOT_RNTuple_Write_ContainerImp::commit not implemented"); } - void ROOT_RNTuple_Write_ContainerImp::setupWrite(std::type_info const& /*type*/) { return; } + void ROOT_RNTuple_Write_ContainerImp::setupWrite(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 8b2e2a6d5..de54fc4ca 100644 --- a/form/root_storage/root_rntuple_write_container.hpp +++ b/form/root_storage/root_rntuple_write_container.hpp @@ -21,10 +21,8 @@ namespace ROOT { class RRawPtrWriteEntry; } #else - namespace Experimental { - namespace Detail { - class RRawPtrWriteEntry; - } + namespace Experimental::Detail { + class RRawPtrWriteEntry; } #endif } diff --git a/phlex/core/index_router.cpp b/phlex/core/index_router.cpp index dbd6d71d7..afd6a39ab 100644 --- a/phlex/core/index_router.cpp +++ b/phlex/core/index_router.cpp @@ -8,6 +8,7 @@ #include "oneapi/tbb/flow_graph.h" #include "spdlog/spdlog.h" +#include #include #include #include @@ -132,9 +133,7 @@ namespace phlex::detail { sorted_layer_paths_ = std::move(layer_paths_from_driver); std::size_t initial_deepest_path_depth = 0; for (auto const& path : sorted_layer_paths_) { - if (path.depth() > initial_deepest_path_depth) { - initial_deepest_path_depth = path.depth(); - } + initial_deepest_path_depth = std::max(path.depth(), initial_deepest_path_depth); } std::size_t const max_allowed_depth = initial_deepest_path_depth + layer_pairs.size(); @@ -466,9 +465,7 @@ namespace phlex::detail { for (auto const& path : sorted_layer_paths_) { if (path.ends_with(name)) { std::size_t const depth = path.depth(); - if (depth > best) { - best = depth; - } + best = std::max(depth, best); } } return best; diff --git a/plugins/layer_generator.cpp b/plugins/layer_generator.cpp index 3544ede99..6bebc67d5 100644 --- a/plugins/layer_generator.cpp +++ b/plugins/layer_generator.cpp @@ -145,7 +145,10 @@ namespace phlex::experimental { }; } - index_generator layer_generator::execute(data_cell_index_ptr const cell) + // Passing the pointer by value is required because this coroutine may outlive its caller; + // prioritize preserving ownership in the coroutine frame over the extra shared_ptr copy. + // NOLINTNEXTLINE(performance-unnecessary-value-param) + index_generator layer_generator::execute(data_cell_index_ptr cell) { // Used in drivers which are close to public API --> easier to stick to strings auto cell_lp = cell->layer_path().to_string(); diff --git a/test/demo-giantdata/waveform_generator.cpp b/test/demo-giantdata/waveform_generator.cpp index b85cd617d..030da769f 100644 --- a/test/demo-giantdata/waveform_generator.cpp +++ b/test/demo-giantdata/waveform_generator.cpp @@ -7,8 +7,6 @@ demo::WaveformGenerator::WaveformGenerator(WGI const& wgi) : maxsize_{wgi.size} {} -demo::WaveformGenerator::~WaveformGenerator() = default; - std::size_t demo::WaveformGenerator::initial_value() const { return 0; } bool demo::WaveformGenerator::predicate(std::size_t made_so_far) const diff --git a/test/demo-giantdata/waveform_generator.hpp b/test/demo-giantdata/waveform_generator.hpp index 07d672d70..56a077100 100644 --- a/test/demo-giantdata/waveform_generator.hpp +++ b/test/demo-giantdata/waveform_generator.hpp @@ -21,7 +21,7 @@ namespace demo { WaveformGenerator(WaveformGenerator&&) = delete; WaveformGenerator& operator=(WaveformGenerator const&) = delete; WaveformGenerator& operator=(WaveformGenerator&&) = delete; - ~WaveformGenerator(); + ~WaveformGenerator() = default; std::size_t initial_value() const; diff --git a/test/flush_gate_test.cpp b/test/flush_gate_test.cpp index dfa9d8e6d..cc3da0f8e 100644 --- a/test/flush_gate_test.cpp +++ b/test/flush_gate_test.cpp @@ -192,7 +192,7 @@ TEST_CASE("flush_gate: two-layer hierarchy (job -> runs -> spills)", "[flush_gat for (auto const& ft : flushed) { if (ft->index()->layer_name() == "run"_id) { CHECK(ft->expected_total_count() == n_spills); - CHECK(ft->committed_count_for_layer(spill_layer_hash) == n_spills); + CHECK(std::cmp_equal(ft->committed_count_for_layer(spill_layer_hash), n_spills)); } else { REQUIRE(ft->index() == job); job_flushed = ft; @@ -202,7 +202,7 @@ TEST_CASE("flush_gate: two-layer hierarchy (job -> runs -> spills)", "[flush_gat REQUIRE(job_flushed); CHECK(job_flushed->expected_total_count() == n_runs); // Immediate children (runs) counted directly. - CHECK(job_flushed->committed_count_for_layer(run_layer_hash) == n_runs); + CHECK(std::cmp_equal(job_flushed->committed_count_for_layer(run_layer_hash), n_runs)); // Grandchildren (spills) propagated up from the run gates. CHECK(job_flushed->committed_count_for_layer(spill_layer_hash) == n_runs * n_spills);