Clang-Tidy: cppcoreguidelines-narrowing-conversions warning and fix#3098
Clang-Tidy: cppcoreguidelines-narrowing-conversions warning and fix#3098Ritesh200422 wants to merge 6 commits intopgRouting:developfrom
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:
WalkthroughRemoved the explicit disable for clang-tidy Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 `@src/cpp_common/undirectedHasCostBG.cpp`:
- Line 169: The current call auto v =
add_vertex(static_cast<int>(m_id_to_V.size()), m_graph); performs a narrowing
cast from size_t to int; add a runtime guard to assert or check that
m_id_to_V.size() <= static_cast<size_t>(std::numeric_limits<int>::max()) before
the static_cast to prevent overflow, and fail-fast (assert/throw/log) if the
check fails—update the code around the add_vertex call (referencing m_id_to_V,
add_vertex, and m_graph) to perform this guard and then perform the safe
static_cast.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54727ade-1bd8-4797-84c6-cc9576beb225
📒 Files selected for processing (2)
.clang-tidysrc/cpp_common/undirectedHasCostBG.cpp
💤 Files with no reviewable changes (1)
- .clang-tidy
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/cpp_common/undirectedHasCostBG.cpp`:
- Around line 35-36: The file undirectedHasCostBG.cpp contains a redundant
duplicate include of <limits>; remove the extra line so only one `#include`
<limits> remains alongside the other headers (the existing include block with
`#include`<cassert> and `#include` <limits>), leaving a single <limits> include to
avoid redundant headers.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 16b38034-4574-4d5a-9d2d-7cbb041974f8
📒 Files selected for processing (1)
src/cpp_common/undirectedHasCostBG.cpp
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/cpp_common/undirectedHasCostBG.cpp (1)
34-34: 🧹 Nitpick | 🔵 TrivialAdd space after
#includefor consistency.The include directive is missing a space, which is inconsistent with the other includes in this file (e.g., line 35).
🧹 Suggested fix
-#include<cassert> +#include <cassert>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpp_common/undirectedHasCostBG.cpp` at line 34, The include directive for the C assert header is missing a space; change the token '#include<cassert>' to use the consistent spacing form '#include <cassert>' so it matches the other includes (replace the include of the cassert header in this file).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/cpp_common/undirectedHasCostBG.cpp`:
- Line 34: The include directive for the C assert header is missing a space;
change the token '#include<cassert>' to use the consistent spacing form
'#include <cassert>' so it matches the other includes (replace the include of
the cassert header in this file).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 15f28932-1f47-4ceb-9c4f-5578b3c059dd
📒 Files selected for processing (1)
src/cpp_common/undirectedHasCostBG.cpp
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/cpp_common/undirectedHasCostBG.cpp`:
- Around line 169-170: The assert before the narrowing cast can be disabled in
release builds, so add an explicit runtime guard that checks m_id_to_V.size() <=
static_cast<size_t>(std::numeric_limits<int>::max()) before calling add_vertex;
if the check fails, handle it deterministically (e.g., throw a
std::runtime_error or call std::abort with a clear message) rather than
performing the static_cast, then proceed to call
add_vertex(static_cast<int>(m_id_to_V.size()), m_graph).
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d2738d9-0754-4f15-9ab4-5fcb1a9cb57f
📒 Files selected for processing (1)
src/cpp_common/undirectedHasCostBG.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cpp_common/undirectedHasCostBG.cpp (1)
171-182: 🧹 Nitpick | 🔵 TrivialThe specific overflow error message is lost to the generic catch-all.
The
std::overflow_error("vertex index overflow")thrown at line 172 will be caught bycatch(...)at line 178, converting it to the generic message"INTERNAL: something went wrong when inserting a vertex". This loses diagnostic specificity.Consider catching
std::overflow_errorexplicitly before the catch-all to preserve the message:♻️ Proposed fix to preserve overflow error context
} catch (const std::overflow_error& e) { + throw std::make_pair( + std::string(e.what()), + std::string(__PGR_PRETTY_FUNCTION__)); } catch (...) { throw std::make_pair( std::string("INTERNAL: something went wrong when inserting a vertex"), std::string(__PGR_PRETTY_FUNCTION__)); }Based on learnings: error messages in pgRouting are verified by pgtap tests. If this error path is exercised by tests, ensure they are updated accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpp_common/undirectedHasCostBG.cpp` around lines 171 - 182, The generic catch-all is swallowing the std::overflow_error("vertex index overflow"); add an explicit catch for std::overflow_error before the catch(...) in the block that calls add_vertex/static_cast<int>(vertex_count) so the original error message is preserved (either rethrow the caught overflow_error or throw a new exception that includes e.what() combined with context like __PGR_PRETTY_FUNCTION__ and the existing "INTERNAL: something went wrong when inserting a vertex" text); keep the catch(...) afterwards for other exceptions. Target symbols: add_vertex, m_id_to_V, m_V_to_id, and __PGR_PRETTY_FUNCTION__.
🤖 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/cpp_common/undirectedHasCostBG.cpp`:
- Line 34: The file still includes the unused header <cassert> even though the
assert macro was removed; delete the line that includes <cassert> (remove the
unused include) and verify there are no remaining uses of assert in
undirectedHasCostBG.cpp so the compilation remains clean.
---
Outside diff comments:
In `@src/cpp_common/undirectedHasCostBG.cpp`:
- Around line 171-182: The generic catch-all is swallowing the
std::overflow_error("vertex index overflow"); add an explicit catch for
std::overflow_error before the catch(...) in the block that calls
add_vertex/static_cast<int>(vertex_count) so the original error message is
preserved (either rethrow the caught overflow_error or throw a new exception
that includes e.what() combined with context like __PGR_PRETTY_FUNCTION__ and
the existing "INTERNAL: something went wrong when inserting a vertex" text);
keep the catch(...) afterwards for other exceptions. Target symbols: add_vertex,
m_id_to_V, m_V_to_id, and __PGR_PRETTY_FUNCTION__.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b310a469-e9e8-47b8-ba80-d91a0b4ddad2
📒 Files selected for processing (1)
src/cpp_common/undirectedHasCostBG.cpp
1 participant
Fixes # .
Changes proposed in this pull request:
@pgRouting/admins
Summary by CodeRabbit