Skip to content

clang-tidy: check and fix cppcoreguidelines-pro-bounds-array-to-pointer-decay#3074

Open
prashsti29 wants to merge 10 commits intopgRouting:developfrom
prashsti29:prashsti29-clang-tidy
Open

clang-tidy: check and fix cppcoreguidelines-pro-bounds-array-to-pointer-decay#3074
prashsti29 wants to merge 10 commits intopgRouting:developfrom
prashsti29:prashsti29-clang-tidy

Conversation

@prashsti29
Copy link

@prashsti29 prashsti29 commented Feb 20, 2026

Summary:

  • Enable cppcoreguidelines-pro-bounds-array-to-pointer-decay in .clang-tidy.
  • Fixed warnings related to implicit array-to-pointer decay.

Changes Made:

  • Fixed implicit array-to-pointer decay in src/breadthFirstSearch/binaryBreadthFirstSearch_driver.cpp by replacing const char c_err_msg[] with const std::string c_err_msg.
  • Fixed implicit array-to-pointer decay in src/common/assert.cpp by adding an explicit static_cast<void**> when passing trace[] to backtrace() and backtrace_symbols().

Summary by CodeRabbit

  • Refactor

    • Enabled an additional static analysis check to tighten code safety.
    • Switched internal error message handling to safer string semantics.
    • Improved stack-trace capture and null-checking to reduce crashes and improve reliability.
  • Chores

    • Minor build hygiene and include adjustments to support the above changes.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 20, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Enabled the clang-tidy check cppcoreguidelines-pro-bounds-array-to-pointer-decay and updated source code to satisfy it: replaced a C-style string with std::string in the BFS driver and switched backtrace/backtrace_symbols usage to std::data() with a null-check in assert.cpp.

Changes

Cohort / File(s) Summary
Linter Configuration
./.clang-tidy
Removed exclusion for cppcoreguidelines-pro-bounds-array-to-pointer-decay, enabling that check.
Breadth-first search driver
src/breadthFirstSearch/binaryBreadthFirstSearch_driver.cpp
Replaced local C-style error message const char c_err_msg[] with const std::string c_err_msg.
Assert / backtrace handling
src/common/assert.cpp
Replaced raw pointer use in backtrace/backtrace_symbols calls with std::data(trace), added null-check for funcNames, adjusted freeing logic, and added <iterator> include.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

Enhancement, C/C++

Suggested reviewers

  • cvvergara
  • robe2

Poem

🐰 A tidy hop through code so bright,
C-strings traded for safer light.
std::data guards the backtrace trail,
Little fixes leave a happy trail.
🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and accurately describes the main changes: enabling a clang-tidy check and fixing the related warnings throughout the codebase.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

You can generate walkthrough in a markdown collapsible section to save space.

Enable the reviews.collapse_walkthrough setting to generate walkthrough in a markdown collapsible section.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 47-48: Replace the explicit static_cast<void**>(trace) uses with
the C++17 idiomatic std::data(trace) when calling backtrace and
backtrace_symbols: update the calls that reference trace
(trace_size/backtrace/backtrace_symbols) to pass std::data(trace) instead of
static_cast, and add the proper include for std::data (e.g., `#include`
<iterator>) so the code compiles.

@cvvergara cvvergara marked this pull request as draft February 21, 2026 00:19
@cvvergara
Copy link
Member

You don't have your actions running

@prashsti29 prashsti29 marked this pull request as ready for review February 21, 2026 02:34
@cvvergara cvvergara marked this pull request as draft March 1, 2026 17:43
Copy link
Member

@cvvergara cvvergara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont see your actions running

@prashsti29 prashsti29 marked this pull request as ready for review March 2, 2026 14:19
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 @.ci-trigger:
- Line 1: Remove the temporary CI trigger file named .ci-trigger from the
repository before merging; delete the file and any references to it (there are
no code symbols to change) so the commit does not include this workaround, and
rely on proper GitHub Actions workflow configuration or the workflow re-run
button instead.

ℹ️ Review info

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between edae3d3 and 8e11980.

📒 Files selected for processing (1)
  • .ci-trigger

.ci-trigger Outdated
@@ -0,0 +1 @@
trigger CI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Remove this CI trigger file before merging.

This file appears to be a workaround to force GitHub Actions to run (per the maintainer's comment about actions not running). It serves no functional purpose in the repository and should be removed before merging to avoid permanent clutter.

Consider using GitHub's workflow re-run button or fixing the underlying CI trigger configuration instead.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.ci-trigger at line 1, Remove the temporary CI trigger file named
.ci-trigger from the repository before merging; delete the file and any
references to it (there are no code symbols to change) so the commit does not
include this workaround, and rely on proper GitHub Actions workflow
configuration or the workflow re-run button instead.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

38-48: ⚠️ Potential issue | 🔴 Critical

Missing #include <iterator> for std::data() with raw arrays.

std::data() applied to a raw C-style array (void *trace[16]) requires the <iterator> header in C++17. Without it, the overload template<class T, std::size_t N> T* data(T (&)[N]) is not declared and the code will fail to compile.

Proposed fix
 `#include` <string>
 `#include` <exception>
+#include <iterator>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/common/assert.cpp` around lines 38 - 48, The build fails because
get_backtrace() uses std::data(trace) on a raw array but the <iterator> header
(which defines std::data overloads for C-style arrays) is not included; fix by
adding `#include` <iterator> at the top of the file so the std::data(trace) call
inside get_backtrace() (and the backtrace/backtrace_symbols usage) compiles
correctly.
🤖 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 38-48: The build fails because get_backtrace() uses
std::data(trace) on a raw array but the <iterator> header (which defines
std::data overloads for C-style arrays) is not included; fix by adding `#include`
<iterator> at the top of the file so the std::data(trace) call inside
get_backtrace() (and the backtrace/backtrace_symbols usage) compiles correctly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: eb5fa478-3759-4700-84b2-349173446893

📥 Commits

Reviewing files that changed from the base of the PR and between 8e11980 and a0fcc65.

📒 Files selected for processing (1)
  • src/common/assert.cpp

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 47-48: The call to backtrace_symbols can return nullptr and the
code unconditionally dereferences funcNames in the loop; update the
assert/reporting code that calls backtrace_symbols(std::data(trace), trace_size)
to check that funcNames is non-null before iterating or dereferencing it (e.g.,
if funcNames == nullptr, log a fallback message or skip symbol names), and
ensure you only call free(funcNames) when funcNames is non-null; reference the
backtrace_symbols result (funcNames), the trace_size value and the loop that
accesses funcNames[i] when making the guard.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 82159482-2d1d-4fb2-8a39-647e6158da33

📥 Commits

Reviewing files that changed from the base of the PR and between a0fcc65 and f96da0b.

📒 Files selected for processing (1)
  • src/common/assert.cpp

@prashsti29
Copy link
Author

hi @cvvergara, could you kindly review this

@prashsti29 prashsti29 requested a review from cvvergara March 13, 2026 22:08
@prashsti29
Copy link
Author

ping @cvvergara

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants