clang-tidy: check and fix cppcoreguidelines-avoid-c-arrays#3084
clang-tidy: check and fix cppcoreguidelines-avoid-c-arrays#3084Cellzawy wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughEnabled the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.clang-tidy:
- Around line 9-10: Remove the redundant explicit rule entry by deleting the
"cppcoreguidelines-avoid-c-arrays" line since "cppcoreguidelines-*" already
enables that rule; locate the block containing "cppcoreguidelines-*" and remove
the separate "cppcoreguidelines-avoid-c-arrays" entry so only the wildcard
remains.
Cellzawy
force-pushed
the
cppcoreguidelines-avoid-c-arrays
branch
2 times, most recently
from
211bbe4 to
efea7a1
Compare
Cellzawy
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/common/assert.cpp (1)
49-55:⚠️ Potential issue | 🟠 MajorGuard
backtrace_symbolsfailure before dereference.On line 49,
backtrace_symbols()can returnnullptron allocation failure. Line 54 unconditionally dereferencesfuncNames[i], which crashes inside exception handling.Suggested fix
- trace_size = backtrace(trace.data(), 16); + trace_size = backtrace(trace.data(), static_cast<int>(trace.size())); char** funcNames = backtrace_symbols(trace.data(), trace_size); + if (funcNames == nullptr) { + return "\n*** Execution path***\n[bt] backtrace_symbols unavailable\n"; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/assert.cpp` around lines 49 - 55, The call to backtrace_symbols may return nullptr and the code unconditionally dereferences funcNames[i]; update the block around backtrace_symbols(trace.data(), trace_size) and the loop that builds message to check if funcNames is non-null before accessing funcNames[i], and if it is null append a safe fallback (e.g., a placeholder line per frame or a single error note) instead of dereferencing; ensure funcNames is freed only if non-null and keep trace_size handling in the loop (symbols from backtrace_symbols, funcNames, trace_size, and message).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/common/assert.cpp`:
- Around line 49-55: The call to backtrace_symbols may return nullptr and the
code unconditionally dereferences funcNames[i]; update the block around
backtrace_symbols(trace.data(), trace_size) and the loop that builds message to
check if funcNames is non-null before accessing funcNames[i], and if it is null
append a safe fallback (e.g., a placeholder line per frame or a single error
note) instead of dereferencing; ensure funcNames is freed only if non-null and
keep trace_size handling in the loop (symbols from backtrace_symbols, funcNames,
trace_size, and message).
ℹ️ Review info
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.clang-tidysrc/common/assert.cpp
💤 Files with no reviewable changes (1)
- .clang-tidy
Cellzawy
force-pushed
the
cppcoreguidelines-avoid-c-arrays
branch
from
efea7a1 to
7726a3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/common/assert.cpp`:
- Line 81: The AssertFailedException constructor currently copies the parameter
msg into member str; change it to move msg into str by adding `#include` <utility>
and using std::move(msg) in the initializer list of
AssertFailedException::AssertFailedException(std::string msg) so the value is
moved into the member (refer to AssertFailedException, its constructor, and
member str).
ℹ️ Review info
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.clang-tidysrc/common/assert.cpp
💤 Files with no reviewable changes (1)
- .clang-tidy
Cellzawy
force-pushed
the
cppcoreguidelines-avoid-c-arrays
branch
from
7726a3f to
95d0119
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/common/assert.cpp`:
- Around line 45-49: Replace the hardcoded literal 16 in the backtrace call with
the std::array's size to avoid duplication: call backtrace(trace.data(),
trace.size()) and keep using trace_size for the returned count
(backtrace_symbols(trace.data(), trace_size)); update any related
comments/assumptions around trace and trace_size if needed so the code relies on
trace.size() rather than the magic number.
ℹ️ Review info
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.clang-tidysrc/common/assert.cpp
💤 Files with no reviewable changes (1)
- .clang-tidy
| std::array<void *, 16> trace; | ||
| int trace_size = 0; | ||
|
|
||
| trace_size = backtrace(trace.data(), 16); | ||
| char **funcNames = backtrace_symbols(trace.data(), trace_size); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good conversion from C-array to std::array; consider using trace.size() to avoid duplication.
The conversion to std::array<void*, 16> correctly addresses the cppcoreguidelines-avoid-c-arrays check. Consider using trace.size() instead of hardcoding 16 again in the backtrace call to avoid duplication and ensure consistency if the size changes.
♻️ Proposed fix
- trace_size = backtrace(trace.data(), 16);
- char **funcNames = backtrace_symbols(trace.data(), trace_size);
+ trace_size = backtrace(trace.data(), trace.size());
+ char **funcNames = backtrace_symbols(trace.data(), trace_size);📝 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.
| std::array<void *, 16> trace; | |
| int trace_size = 0; | |
| trace_size = backtrace(trace.data(), 16); | |
| char **funcNames = backtrace_symbols(trace.data(), trace_size); | |
| std::array<void *, 16> trace; | |
| int trace_size = 0; | |
| trace_size = backtrace(trace.data(), trace.size()); | |
| char **funcNames = backtrace_symbols(trace.data(), trace_size); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/common/assert.cpp` around lines 45 - 49, Replace the hardcoded literal 16
in the backtrace call with the std::array's size to avoid duplication: call
backtrace(trace.data(), trace.size()) and keep using trace_size for the returned
count (backtrace_symbols(trace.data(), trace_size)); update any related
comments/assumptions around trace and trace_size if needed so the code relies on
trace.size() rather than the magic number.
95d0119 to
30f3c84
Compare
|
Fixed warnings raised by checks In
|
|
Your solution was nice, unfortunately a previous PR also solved this problem. |
2 participants
This PR adds
cppcoreguidelines-avoid-c-arrayscheck in clang-tidy (by removing it from the disabled list).Summary by CodeRabbit