clang-tidy: resolve readability-braces-around-statements - #762
Conversation
…ments clang-tidy fixes
|
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:
📝 WalkthroughWalkthroughThis change applies clang-tidy brace fixes across core, storage, Python, and test code. It also makes several Python callback and validation error paths explicit, updates interpreter setup guards, and marks the corresponding documentation checklist item complete. ChangesBraced control flow and Python error handling
Estimated code review effort: 3 (Moderate) | ~20 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: 2
🤖 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 `@plugins/python/src/modulewrap.cpp`:
- Around line 407-422: Update the _sig annotation-processing path to validate
each annotation_as_text result before adding it to the signature types or
setting conversion_ok. After processing ret and args, check PyErr_Occurred()
alongside the returned strings; on any normalization error, return false before
populating signature types, while preserving successful handling of valid empty
strings.
- Around line 413-422: In the normalization path using norm, copy the UTF-8
contents into an owning std::string before decrementing norm. Keep norm alive
through the copy, then release it and return the copied value; preserve the
existing empty-result handling when normalization or UTF-8 conversion fails.
🪄 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: 5a41259e-e96d-479a-a39a-19d168ec2501
📒 Files selected for processing (21)
docs/dev/clang-tidy-fixes-2026-04.mdform/form/config.cppform/form/form_writer.cppform/persistence/persistence_utils.cppform/storage/storage_reader.cppform/storage/storage_writer.cppphlex/app/load_module.cppphlex/model/data_cell_index.cppplugins/python/src/configwrap.cppplugins/python/src/dyncall.cppplugins/python/src/errorwrap.cppplugins/python/src/lifelinewrap.cppplugins/python/src/modulewrap.cppplugins/python/src/pymodule.cpptest/form/form_root_schema_read_test.cpptest/form/form_root_schema_write_test.cpptest/form/form_storage_test.cpptest/form/generate_vector.cpptest/form/reader.cpptest/form/test_utils.hpptest/form/writer.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Analyze cpp with CodeQL
- GitHub Check: build (gcc, none)
- GitHub Check: clang-tidy-check
- GitHub Check: coverage
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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:
plugins/python/src/lifelinewrap.cppplugins/python/src/errorwrap.cpptest/form/form_storage_test.cpptest/form/form_root_schema_read_test.cppphlex/model/data_cell_index.cppform/form/form_writer.cpptest/form/generate_vector.cpptest/form/form_root_schema_write_test.cppform/form/config.cppform/persistence/persistence_utils.cpptest/form/reader.cpptest/form/writer.cppphlex/app/load_module.cppform/storage/storage_writer.cppplugins/python/src/dyncall.cppform/storage/storage_reader.cpptest/form/test_utils.hppplugins/python/src/pymodule.cppplugins/python/src/configwrap.cppplugins/python/src/modulewrap.cpp
**/*.{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:
plugins/python/src/lifelinewrap.cppplugins/python/src/errorwrap.cpptest/form/form_storage_test.cpptest/form/form_root_schema_read_test.cppphlex/model/data_cell_index.cppform/form/form_writer.cpptest/form/generate_vector.cpptest/form/form_root_schema_write_test.cppform/form/config.cppform/persistence/persistence_utils.cpptest/form/reader.cpptest/form/writer.cppphlex/app/load_module.cppform/storage/storage_writer.cppplugins/python/src/dyncall.cppform/storage/storage_reader.cpptest/form/test_utils.hppplugins/python/src/pymodule.cppplugins/python/src/configwrap.cppplugins/python/src/modulewrap.cpp
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/lifelinewrap.cppplugins/python/src/errorwrap.cppplugins/python/src/dyncall.cppplugins/python/src/pymodule.cppplugins/python/src/configwrap.cppplugins/python/src/modulewrap.cpp
**/*.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
**/*.hpp
📄 CodeRabbit inference engine (AGENTS.md)
Avoid boolean parameters in C++ interfaces; prefer enumerations instead
Files:
test/form/test_utils.hpp
🔇 Additional comments (21)
docs/dev/clang-tidy-fixes-2026-04.md (1)
149-150: LGTM!form/form/config.cpp (1)
8-10: LGTM!form/form/form_writer.cpp (1)
50-52: LGTM!form/persistence/persistence_utils.cpp (1)
9-15: LGTM!form/storage/storage_reader.cpp (1)
214-224: LGTM!Also applies to: 291-302, 320-330, 380-391
form/storage/storage_writer.cpp (1)
71-73: LGTM!Also applies to: 97-99
phlex/app/load_module.cpp (1)
71-73: LGTM!phlex/model/data_cell_index.cpp (1)
92-94: LGTM!test/form/form_root_schema_read_test.cpp (1)
21-23: LGTM!test/form/form_root_schema_write_test.cpp (1)
22-24: LGTM!test/form/form_storage_test.cpp (1)
40-42: LGTM!test/form/generate_vector.cpp (1)
24-26: LGTM!test/form/reader.cpp (1)
122-131: LGTM!Also applies to: 177-179
test/form/test_utils.hpp (1)
33-36: LGTM!Also applies to: 92-94
test/form/writer.cpp (1)
85-87: LGTM!Also applies to: 101-103, 113-115, 135-137
plugins/python/src/configwrap.cpp (1)
33-35: LGTM!Also applies to: 95-161
plugins/python/src/dyncall.cpp (1)
17-40: LGTM!Also applies to: 70-96
plugins/python/src/errorwrap.cpp (1)
72-74: LGTM!plugins/python/src/lifelinewrap.cpp (1)
22-24: LGTM!plugins/python/src/modulewrap.cpp (1)
184-199: LGTM!Also applies to: 233-235, 288-295, 315-331, 355-357, 379-387, 495-497, 1049-1051, 1062-1064, 1197-1199, 1210-1212, 1398-1400
plugins/python/src/pymodule.cpp (1)
56-58: LGTM!Also applies to: 95-97, 109-111, 173-193
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #762 +/- ##
==========================================
- Coverage 83.82% 83.79% -0.04%
==========================================
Files 172 172
Lines 7210 7226 +16
Branches 848 848
==========================================
+ Hits 6044 6055 +11
- Misses 884 889 +5
Partials 282 282
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
01e2d2e to
e6a1cec
Compare
I did not notice the issue raised by Beojan.
aolivier23
left a comment
aolivier23
left a comment
There was a problem hiding this comment.
FORM changes look good to me. I checked on the name shadowing in FORM and am convinced it doesn't impact code correctness in this PR. There were parts of the python code I didn't get to.
Separate issue created (#779) that tracks the name-shadowing.
4 participants
Resolve
readability-braces-around-statementsclang-tidy warnings.readability-braces-around-statementsclang-tidy warnings across form, storage, plugin, and test code by adding explicit braces to conditional and loop bodies.voidreturn handling.#762.