Clang-Tidy: cppcoreguidelines-narrowing-conversions warning and fix#3098
Clang-Tidy: cppcoreguidelines-narrowing-conversions warning and fix#3098Ritesh200422 wants to merge 7 commits intopgRouting:developfrom
Conversation
WalkthroughRemoved the explicit disable for clang-tidy 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 `@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
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/cpp_common/undirectedHasCostBG.cpp (1)
170-177:⚠️ Potential issue | 🟡 Minor
std::overflow_erroris immediately masked bycatch (...).At line 171 you throw
std::overflow_error, but line 177 catches everything and replaces it with a generic pair, so the overflow-specific failure is lost.Suggested fix
@@ - } catch (...) { + } catch (const std::overflow_error&) { + throw; + } catch (...) { throw std::make_pair( std::string("INTERNAL: something went wrong when inserting a vertex"), std::string(__PGR_PRETTY_FUNCTION__)); }🤖 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 170 - 177, The thrown std::overflow_error from the vertex index check is being swallowed by a following catch(...); fix by adding a specific catch for std::overflow_error before the catch(...) so the overflow is not masked—e.g., add catch(const std::overflow_error& e) { throw; } (or log and rethrow) immediately after the try block that contains the throw std::overflow_error(...) and before the generic catch(...), keeping existing code that inserts via add_vertex(...) and updates m_id_to_V/m_V_to_id unchanged.
🤖 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`:
- Around line 170-177: The thrown std::overflow_error from the vertex index
check is being swallowed by a following catch(...); fix by adding a specific
catch for std::overflow_error before the catch(...) so the overflow is not
masked—e.g., add catch(const std::overflow_error& e) { throw; } (or log and
rethrow) immediately after the try block that contains the throw
std::overflow_error(...) and before the generic catch(...), keeping existing
code that inserts via add_vertex(...) and updates m_id_to_V/m_V_to_id unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ee40491-142a-45dc-bef0-f18c6410853d
📒 Files selected for processing (1)
src/cpp_common/undirectedHasCostBG.cpp
1 participant
Fixes # .
Changes proposed in this pull request:
@pgRouting/admins
Summary by CodeRabbit