Fix `clang-tidy`-flagged naming issues by greenc-FNAL · Pull Request #362 · Framework-R-D/phlex

greenc-FNAL · 2026-02-25T15:52:06Z

No description provided.

greenc-FNAL · 2026-02-25T15:52:38Z

@phlexbot tidy-fix readability-identifier-naming

greenc-FNAL · 2026-02-25T16:46:12Z

@phlexbot tidy-fix readability-identifier-naming

beojan · 2026-02-25T18:59:49Z

Looks like it's changing the #define of header guards without changing the #ifndef to match.

greenc-FNAL · 2026-02-25T19:58:20Z

Looks like it's changing the #define of header guards without changing the #ifndef to match.

The header guards are a known limitation for clang-tidy, which I address here with 6abc94e and in the workflow PR with a Python helper script.

However, using a pre-generated fixes YAML file when fixing only specific issues can lead to actual inconsistencies in the code, so I will be re-doing the fixes for this PR after resolving the issue in the workflow PR. Thanks for spotting this, @beojan!

codecov · 2026-02-27T16:35:23Z

Codecov Report

❌ Patch coverage is 96.62921% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
phlex/core/declared_fold.hpp	95.65%	1 Missing ⚠️
phlex/core/declared_unfold.hpp	83.33%	1 Missing ⚠️
phlex/core/multilayer_join_node.hpp	75.00%	0 Missing and 1 partial ⚠️

@@            Coverage Diff             @@
##             main     #362      +/-   ##
==========================================
+ Coverage   86.37%   86.43%   +0.05%     
==========================================
  Files         119      119              
  Lines        2400     2403       +3     
  Branches      387      387              
==========================================
+ Hits         2073     2077       +4     
  Misses        207      207              
+ Partials      120      119       -1

Flag	Coverage Δ
unittests	`86.43% <96.62%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
phlex/core/declared_observer.hpp	`100.00% <100.00%> (ø)`
phlex/core/declared_predicate.hpp	`94.73% <100.00%> (ø)`
phlex/core/declared_transform.hpp	`88.57% <100.00%> (+0.33%)`	⬆️
phlex/core/input_arguments.hpp	`100.00% <100.00%> (ø)`
phlex/core/registrar.hpp	`81.25% <100.00%> (ø)`
phlex/core/registration_api.hpp	`100.00% <100.00%> (ø)`
phlex/model/product_store.hpp	`100.00% <100.00%> (ø)`
phlex/model/type_id.hpp	`92.77% <ø> (ø)`
phlex/utilities/simple_ptr_map.hpp	`100.00% <100.00%> (ø)`
phlex/core/declared_fold.hpp	`95.31% <95.65%> (+0.15%)`	⬆️
... and 2 more

... and 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f9bec6...75b2636. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

knoepfel

@greenc-FNAL, all of these changes look consistent with what I would expect. It does illustrate at least one naming issue.

Whereas N and M seemed (to me) to be reasonable identifiers for representing the number of inputs and outputs, respectively, for an algorithm, the identifiers n and m are too easily glossed over.

I think the solution to this problem is to just replace n with num_inputs, and m with num_outputs (or something similar). That should maybe be another PR?

greenc-FNAL · 2026-02-27T19:13:22Z

I think the solution to this problem is to just replace n with num_inputs, and m with num_outputs (or something similar). That should maybe be another PR?

Given that the originals (N, M) were OK, and the ones in this PR are not, I would lean towards changing their names in this PR as a post-fix commit, and mentioning it in the description and squashed commit message. Absolutely willing to go with what you prefer, though.

knoepfel · 2026-02-27T19:15:13Z

I think the solution to this problem is to just replace n with num_inputs, and m with num_outputs (or something similar). That should maybe be another PR?

Given that the originals (N, M) were OK, and the ones in this PR are not, I would lean towards changing their names in this PR as a post-fix commit, and mentioning it in the description and squashed commit message. Absolutely willing to go with what you prefer, though.

Let's fix it here then

Per #362 (review): - `n` -> `num_inputs` - `m` -> `num_outputs`

greenc-FNAL · 2026-02-27T20:58:13Z

@phlexbot clang-fix

greenc-FNAL · 2026-02-27T21:01:28Z

@phlexbot format

Per #362 (review): - `n` -> `num_inputs` - `m` -> `num_outputs`

greenc-FNAL · 2026-02-27T21:46:53Z

@phlexbot format

github-actions · 2026-02-27T21:47:08Z

No automatic header-guards fixes were necessary.

github-actions · 2026-02-27T21:47:08Z

No automatic jsonnetfmt fixes were necessary.

github-actions · 2026-02-27T21:47:10Z

No automatic cmake-format fixes were necessary.

github-actions · 2026-02-27T21:47:14Z

No automatic clang-format fixes were necessary.

github-actions · 2026-02-27T21:47:14Z

No automatic markdownlint fixes were necessary.

knoepfel

Only one request, @greenc-FNAL. Otherwise, looks great.

phlex/core/declared_fold.hpp

Per #362 (comment) Co-authored-by: Kyle Knoepfel <knoepfel@fnal.gov>

Copilot

Pull request overview

Adjusts C++ identifier names to satisfy the repository’s clang-tidy naming rules (notably lower_case for most identifiers and CamelCase for template parameters), primarily within core graph node/registration code and a demo test.

Changes:

Renames various type aliases/constants/template parameters to match readability-identifier-naming settings.
Updates several core node implementations (transform/predicate/observer/unfold/fold) to use the renamed num_inputs/num_outputs constants.
Removes the ENABLE_CLANG_TIDY CMake option and its build-time integration logic.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
test/demo-giantdata/unfold_transform_fold.cpp	Renames local tracker struct/type usage to match naming rules.
phlex/utilities/simple_ptr_map.hpp	Renames internal type alias and updates related signatures.
phlex/model/type_id.hpp	Renames internal signed-type alias to `lower_case`.
phlex/model/product_store.hpp	Renames tuple-size constant to `num_inputs`.
phlex/core/registration_api.hpp	Renames internal type aliases/constants (`node_ptr`, `num_inputs`, `num_outputs`).
phlex/core/registrar.hpp	Renames internal `Nodes` alias to `nodes` and updates member type.
phlex/core/multilayer_join_node.hpp	Renames template parameter to `CamelCase` and updates dependent uses.
phlex/core/input_arguments.hpp	Renames tuple-size constant to `num_inputs`.
phlex/core/declared_unfold.hpp	Renames internal aliases/constants and updates join/call wiring.
phlex/core/declared_transform.hpp	Renames internal constants and updates join/call wiring.
phlex/core/declared_predicate.hpp	Renames internal aliases/constants and updates join/call wiring.
phlex/core/declared_observer.hpp	Renames internal aliases/constants and updates join/call wiring.
phlex/core/declared_fold.hpp	Renames internal constants/type alias and updates join/call wiring (but introduces compile errors).
CMakeLists.txt	Removes `ENABLE_CLANG_TIDY` option and CMake-based clang-tidy integration.

phlex/core/declared_fold.hpp

phlex/utilities/simple_ptr_map.hpp

CMakeLists.txt

github-actions · 2026-03-03T22:41:18Z

Automatic clang-format fixes pushed (commit 5deb57b).
⚠️ Note: Some issues may require manual review and fixing.

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

phlex/utilities/simple_ptr_map.hpp

phlex/core/registrar.hpp

phlex/core/declared_fold.hpp

greenc-FNAL · 2026-03-03T23:22:29Z

@copilot open a new pull request to apply changes based on the comments in this thread

Copilot · 2026-03-03T23:22:36Z

@greenc-FNAL I've opened a new pull request, #381, to work on those changes. Once the pull request is ready, I'll request review from you.

Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>

greenc-FNAL · 2026-03-03T23:33:51Z

@phlexbot clang-fix

github-actions · 2026-03-03T23:34:18Z

No automatic clang-format fixes were necessary.

github-actions · 2026-03-03T23:35:05Z

No automatic clang-tidy fixes were necessary.

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

phlex/core/declared_fold.hpp

- Per #362 (comment) Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

phlex/utilities/simple_ptr_map.hpp:37

The in-class comment still refers to Ptr (e.g., std::map<std::string, Ptr> / std::pair<..., Ptr>), but the alias was renamed to ptr. Updating the comment to match the current alias name would avoid confusion when reading this specialization.

    using ptr = std::unique_ptr<T>;
    std::map<std::string, ptr> data_;

  public:
    // std::map<std::string, Ptr> has a default constructor that does
    // not invoke the deleted default constructor of its
    // std::pair<std::string const, Ptr> data member, but the compiler

- Per #362 (review)

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

phlex/utilities/simple_ptr_map.hpp

- Per #362 (review)

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

greenc-FNAL force-pushed the maintenance/clang-tidy-naming branch 2 times, most recently from e1aa8ad to fae5088 Compare February 25, 2026 18:43

greenc-FNAL force-pushed the maintenance/clang-tidy-naming branch from 6abc94e to 7137a36 Compare February 27, 2026 16:26

greenc-FNAL marked this pull request as ready for review February 27, 2026 16:43

greenc-FNAL requested a review from knoepfel February 27, 2026 16:43

knoepfel reviewed Feb 27, 2026

View reviewed changes

greenc-FNAL added a commit that referenced this pull request Feb 27, 2026

Rename too-short variables

544cac5

Per #362 (review): - `n` -> `num_inputs` - `m` -> `num_outputs`

greenc-FNAL force-pushed the maintenance/clang-tidy-naming branch from 7ef4e8c to 544cac5 Compare February 27, 2026 20:56

greenc-FNAL added a commit that referenced this pull request Feb 27, 2026

Rename too-short variables

53f2a74

Per #362 (review): - `n` -> `num_inputs` - `m` -> `num_outputs`

greenc-FNAL force-pushed the maintenance/clang-tidy-naming branch from 0e54e8f to 626bc70 Compare February 27, 2026 21:41

knoepfel reviewed Mar 3, 2026

View reviewed changes

phlex/core/declared_fold.hpp Outdated Show resolved Hide resolved

greenc-FNAL added a commit that referenced this pull request Mar 3, 2026

using r -> using result_type

8261d21

Per #362 (comment) Co-authored-by: Kyle Knoepfel <knoepfel@fnal.gov>

Copilot AI review requested due to automatic review settings March 3, 2026 22:11

Copilot started reviewing on behalf of greenc-FNAL March 3, 2026 22:11 View session

Copilot AI reviewed Mar 3, 2026

View reviewed changes

phlex/core/declared_fold.hpp Outdated Show resolved Hide resolved

phlex/core/declared_fold.hpp Show resolved Hide resolved

phlex/core/declared_fold.hpp Show resolved Hide resolved

phlex/utilities/simple_ptr_map.hpp Outdated Show resolved Hide resolved

CMakeLists.txt Show resolved Hide resolved

Apply clang-format fixes

5deb57b

Copilot AI reviewed Mar 3, 2026

View reviewed changes

phlex/utilities/simple_ptr_map.hpp Show resolved Hide resolved

phlex/core/registrar.hpp Outdated Show resolved Hide resolved

phlex/core/declared_fold.hpp Show resolved Hide resolved

Copilot AI mentioned this pull request Mar 3, 2026

Fix clang-tidy readability-identifier-naming violations #381

Merged

Fix nodes& nodes parameter shadowing in registrar constructor

63f563c

Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>

greenc-FNAL requested a review from Copilot March 3, 2026 23:38

Copilot started reviewing on behalf of greenc-FNAL March 3, 2026 23:38 View session

Copilot AI reviewed Mar 3, 2026

View reviewed changes

phlex/core/declared_fold.hpp Outdated Show resolved Hide resolved

Fix dangling reference

578bb45

- Per #362 (comment) Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

greenc-FNAL requested a review from Copilot March 3, 2026 23:45

Copilot started reviewing on behalf of greenc-FNAL March 3, 2026 23:45 View session

Copilot AI reviewed Mar 3, 2026

View reviewed changes

greenc-FNAL added a commit that referenced this pull request Mar 4, 2026

Bring comments into line with code

1494b7a

- Per #362 (review)

greenc-FNAL requested review from Copilot and knoepfel March 4, 2026 00:13

Copilot started reviewing on behalf of greenc-FNAL March 4, 2026 00:13 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

phlex/utilities/simple_ptr_map.hpp Outdated Show resolved Hide resolved

Bring comments into line with code

75b2636

- Per #362 (review)

greenc-FNAL force-pushed the maintenance/clang-tidy-naming branch from 1494b7a to 75b2636 Compare March 4, 2026 00:20

greenc-FNAL requested a review from Copilot March 4, 2026 00:20

Copilot started reviewing on behalf of greenc-FNAL March 4, 2026 00:21 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

knoepfel approved these changes Mar 4, 2026

View reviewed changes

knoepfel merged commit b8c13f6 into main Mar 4, 2026
65 checks passed

knoepfel deleted the maintenance/clang-tidy-naming branch March 4, 2026 14:31

Conversation

greenc-FNAL commented Feb 25, 2026

Uh oh!

greenc-FNAL commented Feb 25, 2026

Uh oh!

greenc-FNAL commented Feb 25, 2026

Uh oh!

beojan commented Feb 25, 2026

Uh oh!

greenc-FNAL commented Feb 25, 2026

Uh oh!

codecov bot commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

knoepfel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greenc-FNAL commented Feb 27, 2026

Uh oh!

knoepfel commented Feb 27, 2026

Uh oh!

greenc-FNAL commented Feb 27, 2026

Uh oh!

greenc-FNAL commented Feb 27, 2026

Uh oh!

greenc-FNAL commented Feb 27, 2026

Uh oh!

github-actions bot commented Feb 27, 2026

Uh oh!

github-actions bot commented Feb 27, 2026

Uh oh!

github-actions bot commented Feb 27, 2026

Uh oh!

github-actions bot commented Feb 27, 2026

Uh oh!

github-actions bot commented Feb 27, 2026

Uh oh!

knoepfel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Mar 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greenc-FNAL commented Mar 3, 2026

Uh oh!

Copilot AI commented Mar 3, 2026

Uh oh!

greenc-FNAL commented Mar 3, 2026

Uh oh!

github-actions bot commented Mar 3, 2026

Uh oh!

github-actions bot commented Mar 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

codecov bot commented Feb 27, 2026 •

edited

Loading

knoepfel left a comment •

edited

Loading