clang-tidy: resolve readability-braces-around-statements#762
Conversation
📝 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
| if (!normalizer) { | ||
| return ""; | ||
| } | ||
| // LCOV_EXCL_STOP | ||
| } | ||
|
|
||
| PyObject* norm = PyObject_CallOneArg(normalizer, pyobj); | ||
| if (!norm) | ||
| if (!norm) { | ||
| return ""; | ||
| } | ||
|
|
||
| char const* ann = PyUnicode_AsUTF8(norm); | ||
| Py_DECREF(norm); | ||
| if (!ann) | ||
| if (!ann) { | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C 8 'annotation_as_text|conversion_ok = true' plugins/python/src/modulewrap.cppRepository: Framework-R-D/phlex
Length of output: 2231
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l plugins/python/src/modulewrap.cpp
sed -n '418,455p' plugins/python/src/modulewrap.cpp
sed -n '430,465p' plugins/python/src/modulewrap.cpp
sed -n '420,455p' plugins/python/src/modulewrap.cppRepository: Framework-R-D/phlex
Length of output: 4283
Preserve annotation-normalization errors on the _sig path.
In plugins/python/src/modulewrap.cpp lines 447-452, annotation_as_text(ret/args) returns "" on Python exception/error, but this branch then pushes those empty strings and sets conversion_ok = true without checking PyErr_Occurred(). Check each returned string and return false before populating the signature types so registration doesn’t continue with invalid annotations while a Python exception stays pending.
🤖 Prompt for 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.
In `@plugins/python/src/modulewrap.cpp` around lines 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.
Source: Coding guidelines
| PyObject* norm = PyObject_CallOneArg(normalizer, pyobj); | ||
| if (!norm) | ||
| if (!norm) { | ||
| return ""; | ||
| } | ||
|
|
||
| char const* ann = PyUnicode_AsUTF8(norm); | ||
| Py_DECREF(norm); | ||
| if (!ann) | ||
| if (!ann) { | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C 3 'PyUnicode_AsUTF8\(norm\)|Py_DECREF\(norm\)|return ann' plugins/python/src/modulewrap.cppRepository: Framework-R-D/phlex
Length of output: 413
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '390,435p' plugins/python/src/modulewrap.cpp
printf '\n--- return/annotations usage nearby ---\n'
rg -n -C 2 'annotation_as_text|\.as_string|conversion_ok|_sig|annotate|as_string' plugins/python/src/modulewrap.cppRepository: Framework-R-D/phlex
Length of output: 3082
🌐 Web query:
Python C API PyUnicode_AsUTF8 object owned by PyObject decref documentation
💡 Result:
The Python C API function PyUnicode_AsUTF8 returns a pointer to a cached, read-only UTF-8 representation of the provided Unicode object [1][2]. Regarding object ownership and memory management: 1. Ownership: The caller does not own the returned pointer and is not responsible for deallocating the memory [1][2]. 2. Deallocation: You must not call Py_DECREF or any other deallocation function on the returned pointer [1][2]. The buffer is owned by the underlying Unicode object and is automatically deallocated when that Unicode object is garbage collected [1][2]. 3. Invalidity: Because the buffer is tied to the lifecycle of the Unicode object, the pointer becomes invalid once the Unicode object is garbage collected [1][2]. If the Unicode object is released (e.g., via Py_DECREF) before you finish using the returned buffer, accessing that buffer results in use-after-free behavior. In summary, the returned pointer is a "borrowed" view of memory managed by the Python object itself [1][3]. Do not attempt to modify this memory or manage its lifecycle manually [1][3].
Citations:
- 1: https://docs.python.org/3/c-api/unicode.html
- 2: https://docs.python.org/3/_sources/c-api/unicode.rst.txt
- 3: Make PyUnicode_AsUTF8 returning "const char *" rather of "char *" python/cpython#72955
Copy the normalized annotation while norm is still alive.
PyUnicode_AsUTF8(norm) gives you a pointer managed by the norm Unicode object, so decrementing norm before returning/copying can make the returned std::string point to freed memory — the same kind of “dangling reference” you’d avoid with any borrowed buffer.
Suggested fix
char const* ann = PyUnicode_AsUTF8(norm);
-Py_DECREF(norm);
if (!ann) {
+ Py_DECREF(norm);
return "";
}
- return ann;
+std::string result{ann};
+Py_DECREF(norm);
+return result;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| PyObject* norm = PyObject_CallOneArg(normalizer, pyobj); | |
| if (!norm) | |
| if (!norm) { | |
| return ""; | |
| } | |
| char const* ann = PyUnicode_AsUTF8(norm); | |
| Py_DECREF(norm); | |
| if (!ann) | |
| if (!ann) { | |
| return ""; | |
| } | |
| PyObject* norm = PyObject_CallOneArg(normalizer, pyobj); | |
| if (!norm) { | |
| return ""; | |
| } | |
| char const* ann = PyUnicode_AsUTF8(norm); | |
| if (!ann) { | |
| Py_DECREF(norm); | |
| return ""; | |
| } | |
| std::string result{ann}; | |
| Py_DECREF(norm); | |
| return result; |
🤖 Prompt for 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.
In `@plugins/python/src/modulewrap.cpp` around lines 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.
Source: Coding guidelines
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (50.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## main #762 +/- ##
==========================================
- Coverage 83.88% 83.83% -0.05%
==========================================
Files 172 172
Lines 7203 7219 +16
Branches 845 845
==========================================
+ Hits 6042 6052 +10
- Misses 882 887 +5
- Partials 279 280 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
1 participant
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.