clang-tidy: resolve performance-unnecessary-value-param - #796
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR completes a clang-tidy cleanup across framework APIs, routing, plugin interfaces, model objects, and Python wrappers. It replaces unnecessary value parameters with references or ChangesClang-tidy value-parameter cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@phlex/core/graph_proxy.hpp`:
- Around line 99-100: Make both the declaration and definition of
graph_proxy::output const, matching the other registration methods and allowing
calls through const graph_proxy<T> instances while preserving its existing
create_glue()-based behavior.
In `@phlex/detail/plugin_macros.hpp`:
- Around line 16-19: The plugin registration macros must preserve the exported
create_module/create_driver ABI by declaring proxy parameters by value, while
passing those parameters by const reference only when invoking the generated
implementation. Update PHLEX_DETAIL_REGISTER_PLUGIN and
PHLEX_DETAIL_REGISTER_DRIVER_PLUGIN accordingly, keeping the
PHLEX_DETAIL_CREATE_2ARGS implementation signature and loader-compatible
exported symbols consistent.
In `@plugins/python/src/modulewrap.cpp`:
- Around line 68-89: The wrappers wrap_module() and wrap_source() currently
expose const PyObject pointers at the Python C API boundary. Change both return
types and their local PyObject wrapper variables to mutable PyObject* while
keeping ph_module and ph_source members const, so callers such as
pymodule_register_providers(), PyObject_CallFunctionObjArgs(), and
PyObject_TypeCheck() receive the expected type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7993b146-d93a-4ca9-b8b5-b2b1ce13eb4e
📒 Files selected for processing (33)
.clang-tidydocs/dev/clang-tidy-fixes-2026-04.mdform/storage/storage_associative_write_container.cppphlex/app/load_module.cppphlex/core/framework_graph.cppphlex/core/framework_graph.hppphlex/core/glue.cppphlex/core/glue.hppphlex/core/graph_proxy.hppphlex/core/index_router.cppphlex/core/index_router.hppphlex/core/make_computational_edges.cppphlex/core/registration_api.cppphlex/core/registration_api.hppphlex/core/store_counters.cppphlex/core/store_counters.hppphlex/detail/plugin_macros.hppphlex/driver.hppphlex/metaprogramming/delegate.hppphlex/model/data_layer_hierarchy.cppphlex/model/data_layer_hierarchy.hppphlex/model/fixed_hierarchy.cppphlex/model/fixed_hierarchy.hppphlex/model/flush_gate.cppphlex/model/flush_gate.hppphlex/module.hppphlex/source.hppplugins/layer_generator.cppplugins/layer_generator.hppplugins/python/src/modulewrap.cppplugins/python/src/pymodule.cppplugins/python/src/wrap.hpptest/flush_gate_test.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.md: All Markdown files must follow markdownlint rule MD012: no multiple consecutive blank lines (never more than one blank line in a row)
All Markdown files must follow markdownlint rule MD022: headings must be surrounded by exactly one blank line before and after
All Markdown files must follow markdownlint rule MD031: fenced code blocks must be surrounded by exactly one blank line before and after
All Markdown files must follow markdownlint rule MD032: lists must be surrounded by exactly one blank line before and after (including after headings and code blocks)
All Markdown files must follow markdownlint rule MD034: no bare URLs (use markdown link syntax like[text](destination)instead of plain URLs)
All Markdown files must follow markdownlint rule MD036: use # headings for titles, not Bold:
All Markdown files must follow markdownlint rule MD040: always specify code block language (for example, use 'bash', 'python', '```text', etc.)
**/*.md: Do not use multiple consecutive blank lines in Markdown (MD012)
Surround Markdown headings with exactly one blank line (MD022)
Surround Markdown fenced code blocks with exactly one blank line (MD031)
Surround Markdown lists with exactly one blank line (MD032)
Do not use bare URLs in Markdown; use[text](url)syntax instead (MD034)
Use#headings in Markdown, not**Bold**for section titles (MD036)
Always specify language on fenced code blocks in Markdown (MD040)
Files:
docs/dev/clang-tidy-fixes-2026-04.md
**/*.{cpp,cc,cxx,h,hpp}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{cpp,cc,cxx,h,hpp}: Use clang-format tool for all C++ code formatting (VS Code auto-formats on save); configuration defined in.clang-formatwith 100-character line limit and 2-space indentation
Follow clang-tidy recommendations defined in.clang-tidy
Files:
phlex/core/store_counters.hppform/storage/storage_associative_write_container.cppphlex/core/store_counters.cppphlex/core/make_computational_edges.cppplugins/layer_generator.hppplugins/python/src/wrap.hppphlex/driver.hppplugins/layer_generator.cppphlex/core/glue.cppphlex/model/flush_gate.cppphlex/core/index_router.hppplugins/python/src/modulewrap.cppphlex/detail/plugin_macros.hppphlex/app/load_module.cppphlex/core/framework_graph.cppplugins/python/src/pymodule.cppphlex/model/fixed_hierarchy.cpptest/flush_gate_test.cppphlex/model/fixed_hierarchy.hppphlex/core/registration_api.cppphlex/module.hppphlex/model/data_layer_hierarchy.hppphlex/model/data_layer_hierarchy.cppphlex/core/graph_proxy.hppphlex/metaprogramming/delegate.hppphlex/core/index_router.cppphlex/source.hppphlex/core/framework_graph.hppphlex/core/glue.hppphlex/model/flush_gate.hppphlex/core/registration_api.hpp
**/*.{hpp,cpp}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{hpp,cpp}: Use.hppfor header files,.cppfor implementation, and*_test.cppfor test files in C++
Enforce 100-character line limit and 2-space indentation in C++ code via.clang-format
UseQualifierAlignment: Right(east-const) style:int const xnotconst int xin C++
UsePointerAlignment: Leftin C++ (pointer*attached to type, not variable name)
All C++ identifiers must uselower_casenaming: namespaces, classes, structs, enums, functions, variables, parameters, members, and constants
Exception to C++ naming: template parameters useCamelCase
Exception to C++ naming: macros useUPPER_CASE
Private, protected, and constant members in C++ must have a trailing underscore (_), no trailing underscore on anything else
Useenum classpreferred over plainenumin C++
Usestd::shared_ptrfor shared ownership,std::unique_ptrfor exclusive ownership, raw pointers for non-owning references only in C++
Use functors with agent-noun pattern:ModelEvaluator evaluate_model(...)in C++
Apply.clang-tidychecks for bugprone, cert, clang-analyzer, concurrency, cppcoreguidelines, misc, modernize, performance, portability, and readability as defined in the.clang-tidyconfiguration file
Usephlex::namespace for core code,phlex::experimental::for experimental features in C++
Files:
phlex/core/store_counters.hppform/storage/storage_associative_write_container.cppphlex/core/store_counters.cppphlex/core/make_computational_edges.cppplugins/layer_generator.hppplugins/python/src/wrap.hppphlex/driver.hppplugins/layer_generator.cppphlex/core/glue.cppphlex/model/flush_gate.cppphlex/core/index_router.hppplugins/python/src/modulewrap.cppphlex/detail/plugin_macros.hppphlex/app/load_module.cppphlex/core/framework_graph.cppplugins/python/src/pymodule.cppphlex/model/fixed_hierarchy.cpptest/flush_gate_test.cppphlex/model/fixed_hierarchy.hppphlex/core/registration_api.cppphlex/module.hppphlex/model/data_layer_hierarchy.hppphlex/model/data_layer_hierarchy.cppphlex/core/graph_proxy.hppphlex/metaprogramming/delegate.hppphlex/core/index_router.cppphlex/source.hppphlex/core/framework_graph.hppphlex/core/glue.hppphlex/model/flush_gate.hppphlex/core/registration_api.hpp
**/*.hpp
📄 CodeRabbit inference engine (AGENTS.md)
Avoid boolean parameters in C++ interfaces; prefer enumerations instead
Files:
phlex/core/store_counters.hppplugins/layer_generator.hppplugins/python/src/wrap.hppphlex/driver.hppphlex/core/index_router.hppphlex/detail/plugin_macros.hppphlex/model/fixed_hierarchy.hppphlex/module.hppphlex/model/data_layer_hierarchy.hppphlex/core/graph_proxy.hppphlex/metaprogramming/delegate.hppphlex/source.hppphlex/core/framework_graph.hppphlex/core/glue.hppphlex/model/flush_gate.hppphlex/core/registration_api.hpp
plugins/python/**/*.cpp
📄 CodeRabbit inference engine (AGENTS.md)
plugins/python/**/*.cpp: Usestd::runtime_errorfor C++ runtime failures; propagate Python exceptions viaPyErr_SetString/PyErr_Format; returnnullptron error; callPyErr_Clear()when recovering in Python/C++ integration
Use manualPy_INCREF/Py_DECREFfor reference counting andPyGILRAIIRAII wrapper for GIL management in C++ code that interacts with Python
For GC-tracked Python types in C++: usePy_TPFLAGS_HAVE_GC, implementtp_traverseandtp_clear, callPyObject_GC_UnTrackbefore deallocation
Files:
plugins/python/src/modulewrap.cppplugins/python/src/pymodule.cpp
🪛 Cppcheck (2.21.0)
phlex/core/glue.cpp
[style] 9-9: The function 'verify_name' is never used.
(unusedFunction)
plugins/python/src/modulewrap.cpp
[style] 68-68: The function 'wrap_module' is never used.
(unusedFunction)
[style] 84-84: The function 'wrap_source' is never used.
(unusedFunction)
🔇 Additional comments (36)
plugins/python/src/modulewrap.cpp (2)
758-758: LGTM!
64-65: 🎯 Functional CorrectnessConst-qualified proxy storage is already safe.
transform(...)andprovide(...)areconst-qualified, so the const pointers can dereference them without compile failures.phlex/core/framework_graph.hpp (1)
31-31: LGTM!Also applies to: 60-64, 81-84, 92-143, 192-194
phlex/core/glue.cpp (1)
6-24: LGTM!phlex/core/glue.hpp (3)
18-18: LGTM!Also applies to: 29-29
175-183: LGTM!add_sourcecorrectly materializes an owningstd::string{name}before it goes intonodes_.sourcesor the error message, so no dangling-view risk here.
56-173: 🩺 Stability & AvailabilityNo change needed for
namelifetime.
registration_api,fold_api,provider_api,unfold_api, andoutputall convertstd::string_view nameintophlex::experimental::algorithm_namebefore storing it.algorithm_nameowns its name components, so these views do not outlive the caller.phlex/core/graph_proxy.hpp (1)
16-16: LGTM!Also applies to: 51-98, 102-115, 138-205, 211-233
phlex/module.hpp (1)
24-29: LGTM!phlex/source.hpp (1)
40-45: LGTM!phlex/core/store_counters.cpp (1)
12-25: LGTM!phlex/core/store_counters.hpp (1)
20-20: LGTM!phlex/model/flush_gate.cpp (1)
43-49: LGTM!phlex/core/registration_api.hpp (1)
19-19: LGTM!Also applies to: 41-48, 62-62, 105-112, 122-129, 150-163, 189-189, 198-198, 266-266, 276-276, 332-332
phlex/core/registration_api.cpp (1)
7-11: LGTM!Also applies to: 21-21
phlex/core/framework_graph.cpp (1)
46-46: LGTM!Also applies to: 85-85, 216-222, 251-251
phlex/core/index_router.cpp (1)
92-93: LGTM!Also applies to: 126-126, 150-153, 193-193, 324-324, 416-418
phlex/core/index_router.hpp (1)
57-57: LGTM!Also applies to: 72-73, 100-100
phlex/core/make_computational_edges.cpp (1)
42-42: LGTM!Also applies to: 71-71, 205-208
phlex/model/flush_gate.hpp (1)
69-69: LGTM!test/flush_gate_test.cpp (1)
28-28: LGTM!Also applies to: 71-73, 228-234
form/storage/storage_associative_write_container.cpp (1)
5-6: LGTM!Also applies to: 31-31
phlex/app/load_module.cpp (1)
53-53: LGTM!phlex/detail/plugin_macros.hpp (1)
44-47: LGTM!Also applies to: 67-72
phlex/metaprogramming/delegate.hpp (1)
8-29: LGTM!Also applies to: 40-51
phlex/model/data_layer_hierarchy.cpp (1)
78-78: LGTM!phlex/model/data_layer_hierarchy.hpp (1)
41-41: LGTM!docs/dev/clang-tidy-fixes-2026-04.md (1)
153-154: LGTM!phlex/driver.hpp (1)
139-139: LGTM!plugins/layer_generator.cpp (1)
39-39: LGTM!plugins/layer_generator.hpp (1)
66-66: LGTM!phlex/model/fixed_hierarchy.cpp (3)
20-22: LGTM!
74-84: LGTM!
115-117: LGTM!phlex/model/fixed_hierarchy.hpp (1)
38-41: LGTM!.clang-tidy (1)
101-101: LGTM!
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #796 +/- ##
==========================================
- Coverage 83.84% 83.83% -0.01%
==========================================
Files 172 172
Lines 7230 7226 -4
Branches 850 850
==========================================
- Hits 6062 6058 -4
Misses 885 885
Partials 283 283
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
1 participant
Resolve
performance-unnecessary-value-paramclang-tidy warnings.Code quality
performance-unnecessary-value-paramclang-tidy warnings.const&orstd::string_view.API and framework
const.flush_gate::roll_up_childto accept counts by reference.Correctness
data_cell_cursorto use non-owning pointers consistently.Tests and documentation
data_cell_countsvalues.docs/dev/clang-tidy-fixes-2026-04.md.