Using enumeration on existing drivers #3050
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughThis pull request replaces hard-coded integer constants with the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ordering/ordering_driver.cpp (1)
103-108:⚠️ Potential issue | 🟠 MajorReplace magic numbers with enum constants and ensure all ordering modes extract vertices.
The condition
which == 0 || which == 2maps toSLOANandKINGonly, excludingCUTCHILL(enum value 1) from vertex extraction. Since all three modes use the undirected graph,CUTCHILLincorrectly receives an empty vertex set, breaking the Cuthill-McKee algorithm. Use enum constants for clarity and extract vertices for all ordering modes:const auto vertices = pgrouting::extract_vertices(edges);
🤖 Fix all issues with AI agents
In `@NEWS.md`:
- Line 15: Replace the bold label "**Code enhancements**" in NEWS.md with a
proper Markdown heading (e.g., an H2) so the generator emits a heading instead
of a bold inline label to satisfy MD036; update the generator/template that
produces the "**Code enhancements**" token (or the NEWS.md source) so the output
uses a heading element for "Code enhancements" rather than bold text.
- Around line 15-17: The NEWS.md entry was edited directly but that file is
generated; revert the manual edit in NEWS.md and instead add the release note to
the generator inputs (or the generator script) so it is preserved on
regeneration: update the notes2news.pl input source or the notes2news.pl logic
to include the “Use enumeration on drivers and process” item (reference
notes2news.pl and the NEWS.md entry text) and then re-run the release-notes
generation to produce an updated NEWS.md.
In `@src/cpp_common/utilities.cpp`:
- Around line 25-29: The code uses std::make_pair in utilities.cpp (around the
code that references make_pair, e.g., functions or lines using make_pair) but
lacks the <utility> header; add `#include` <utility> to the includes at the top of
src/cpp_common/utilities.cpp so the compiler can resolve std::make_pair (ensure
it’s placed alongside other standard headers such as <string>).
- Around line 33-62: In get_name(Which which) remove unreachable code by
deleting all break; statements that follow return in each case of the switch and
also remove the redundant return "unknown"; after the switch; ensure the
function returns the appropriate string from each case and the default case
remains returning "unknown" so there is a single return path per switch branch.
- Around line 87-108: In estimate_drivingSide, avoid undefined behavior by
calling tolower with an unsigned char cast: replace tolower(driving_side) with
tolower(static_cast<unsigned char>(driving_side)); also remove the unreachable
"break;" after the "return ' ';" in the DIJKSTRA case; ensure the function still
handles WHICH cases DIJKSTRA, WITHPOINTS and default as before (no other logic
changes).
In `@src/metrics/metrics_driver.cpp`:
- Around line 76-83: The switch in metrics_driver.cpp handling the Which enum
currently returns 0 silently in the default branch; update the default case in
the function containing the switch (the branch handling BANDWIDTH ->
pgrouting::metrics::bandwidth(undigraph)) to emit a defensive warning or error
log that includes the unexpected Which value (or call site) before returning, or
alternatively add an assertion/exception if unhandled values are programming
errors, so unexpected enum values are visible in logs.
| [#4.1.0](https://github.com/pgRouting/pgrouting/issues?utf8=%E2%9C%93&q=milestone%3A%22Release%204.1.0%22) | ||
|
|
||
| No changes yet | ||
| **Code enhancements** |
There was a problem hiding this comment.
Replace the bold label with a heading to satisfy MD036.
This should be emitted by the generator/source, not edited here.
🛠️ Suggested output change
-**Code enhancements**
+#### Code enhancements📝 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.
| **Code enhancements** | |
| #### Code enhancements |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 15-15: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🤖 Prompt for AI Agents
In `@NEWS.md` at line 15, Replace the bold label "**Code enhancements**" in
NEWS.md with a proper Markdown heading (e.g., an H2) so the generator emits a
heading instead of a bold inline label to satisfy MD036; update the
generator/template that produces the "**Code enhancements**" token (or the
NEWS.md source) so the output uses a heading element for "Code enhancements"
rather than bold text.
| #include "cpp_common/utilities.hpp" | ||
|
|
||
| #include <string> | ||
|
|
||
| #include "c_common/enums.h" |
There was a problem hiding this comment.
Add missing #include <utility> for std::make_pair.
The pipeline is failing because std::make_pair is used at line 97 but <utility> is not included.
🔧 Proposed fix
`#include` "cpp_common/utilities.hpp"
`#include` <string>
+#include <utility>
`#include` "c_common/enums.h"📝 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.
| #include "cpp_common/utilities.hpp" | |
| #include <string> | |
| #include "c_common/enums.h" | |
| `#include` "cpp_common/utilities.hpp" | |
| `#include` <string> | |
| `#include` <utility> | |
| `#include` "c_common/enums.h" |
🧰 Tools
🪛 Cppcheck (2.19.0)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
🤖 Prompt for AI Agents
In `@src/cpp_common/utilities.cpp` around lines 25 - 29, The code uses
std::make_pair in utilities.cpp (around the code that references make_pair,
e.g., functions or lines using make_pair) but lacks the <utility> header; add
`#include` <utility> to the includes at the top of src/cpp_common/utilities.cpp so
the compiler can resolve std::make_pair (ensure it’s placed alongside other
standard headers such as <string>).
| std::string | ||
| get_name(Which which) { | ||
| switch (which) { | ||
| case SLOAN: | ||
| return "pgr_sloanOrdering"; | ||
| break; | ||
| case CUTCHILL: | ||
| return "pgr_cuthillMckeeOrdering"; | ||
| break; | ||
| case KING: | ||
| return "pgr_kingOrdering"; | ||
| break; | ||
| case TOPOSORT: | ||
| return "pgr_topologicalSort"; | ||
| break; | ||
| case FLOYD: | ||
| return "pgr_floydWarshall"; | ||
| break; | ||
| case JOHNSON: | ||
| return "pgr_johnson"; | ||
| break; | ||
| case BANDWIDTH: | ||
| return "pgr_bandwidth"; | ||
| break; | ||
| default: | ||
| return "unknown"; | ||
| break; | ||
| } | ||
| return "unknown"; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unreachable code after return statements.
The break statements after each return and the final return "unknown" on line 61 are unreachable.
♻️ Proposed cleanup
std::string
get_name(Which which) {
switch (which) {
case SLOAN:
return "pgr_sloanOrdering";
- break;
case CUTCHILL:
return "pgr_cuthillMckeeOrdering";
- break;
case KING:
return "pgr_kingOrdering";
- break;
case TOPOSORT:
return "pgr_topologicalSort";
- break;
case FLOYD:
return "pgr_floydWarshall";
- break;
case JOHNSON:
return "pgr_johnson";
- break;
case BANDWIDTH:
return "pgr_bandwidth";
- break;
default:
return "unknown";
- break;
}
- return "unknown";
}📝 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::string | |
| get_name(Which which) { | |
| switch (which) { | |
| case SLOAN: | |
| return "pgr_sloanOrdering"; | |
| break; | |
| case CUTCHILL: | |
| return "pgr_cuthillMckeeOrdering"; | |
| break; | |
| case KING: | |
| return "pgr_kingOrdering"; | |
| break; | |
| case TOPOSORT: | |
| return "pgr_topologicalSort"; | |
| break; | |
| case FLOYD: | |
| return "pgr_floydWarshall"; | |
| break; | |
| case JOHNSON: | |
| return "pgr_johnson"; | |
| break; | |
| case BANDWIDTH: | |
| return "pgr_bandwidth"; | |
| break; | |
| default: | |
| return "unknown"; | |
| break; | |
| } | |
| return "unknown"; | |
| } | |
| std::string | |
| get_name(Which which) { | |
| switch (which) { | |
| case SLOAN: | |
| return "pgr_sloanOrdering"; | |
| case CUTCHILL: | |
| return "pgr_cuthillMckeeOrdering"; | |
| case KING: | |
| return "pgr_kingOrdering"; | |
| case TOPOSORT: | |
| return "pgr_topologicalSort"; | |
| case FLOYD: | |
| return "pgr_floydWarshall"; | |
| case JOHNSON: | |
| return "pgr_johnson"; | |
| case BANDWIDTH: | |
| return "pgr_bandwidth"; | |
| default: | |
| return "unknown"; | |
| } | |
| } |
🧰 Tools
🪛 Cppcheck (2.19.0)
[style] 34-34: The function 'get_name' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
In `@src/cpp_common/utilities.cpp` around lines 33 - 62, In get_name(Which which)
remove unreachable code by deleting all break; statements that follow return in
each case of the switch and also remove the redundant return "unknown"; after
the switch; ensure the function returns the appropriate string from each case
and the default case remains returning "unknown" so there is a single return
path per switch branch.
| char | ||
| estimate_drivingSide(char driving_side, Which which) { | ||
| char d_side = static_cast<char>(tolower(driving_side)); | ||
| if (!((d_side == 'r') || (d_side == 'l') || (d_side == 'b'))) { | ||
| d_side = ' '; | ||
| } | ||
| switch (which) { | ||
| case DIJKSTRA: { return ' '; break; } | ||
| case WITHPOINTS: { | ||
| if (d_side == ' ') { | ||
| throw std::make_pair(std::string("Invalid value of 'driving side'"), | ||
| std::string("Valid value are 'r', 'l', 'b'")); | ||
| } | ||
| break; | ||
| } | ||
| default: { | ||
| /* For the moment its old signature of pgr_withPoints */ | ||
| if (!((d_side == 'r') || (d_side == 'l'))) d_side = 'b'; | ||
| } | ||
| } | ||
| return d_side; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider casting to unsigned char for tolower and remove unreachable break.
Line 89: Passing a char directly to tolower can cause undefined behavior if char is signed and contains a negative value. Line 94 has an unreachable break after return.
♻️ Proposed fix
char
estimate_drivingSide(char driving_side, Which which) {
- char d_side = static_cast<char>(tolower(driving_side));
+ char d_side = static_cast<char>(tolower(static_cast<unsigned char>(driving_side)));
if (!((d_side == 'r') || (d_side == 'l') || (d_side == 'b'))) {
d_side = ' ';
}
switch (which) {
- case DIJKSTRA: { return ' '; break; }
+ case DIJKSTRA: { return ' '; }
case WITHPOINTS: {📝 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.
| char | |
| estimate_drivingSide(char driving_side, Which which) { | |
| char d_side = static_cast<char>(tolower(driving_side)); | |
| if (!((d_side == 'r') || (d_side == 'l') || (d_side == 'b'))) { | |
| d_side = ' '; | |
| } | |
| switch (which) { | |
| case DIJKSTRA: { return ' '; break; } | |
| case WITHPOINTS: { | |
| if (d_side == ' ') { | |
| throw std::make_pair(std::string("Invalid value of 'driving side'"), | |
| std::string("Valid value are 'r', 'l', 'b'")); | |
| } | |
| break; | |
| } | |
| default: { | |
| /* For the moment its old signature of pgr_withPoints */ | |
| if (!((d_side == 'r') || (d_side == 'l'))) d_side = 'b'; | |
| } | |
| } | |
| return d_side; | |
| } | |
| char | |
| estimate_drivingSide(char driving_side, Which which) { | |
| char d_side = static_cast<char>(tolower(static_cast<unsigned char>(driving_side))); | |
| if (!((d_side == 'r') || (d_side == 'l') || (d_side == 'b'))) { | |
| d_side = ' '; | |
| } | |
| switch (which) { | |
| case DIJKSTRA: { return ' '; } | |
| case WITHPOINTS: { | |
| if (d_side == ' ') { | |
| throw std::make_pair(std::string("Invalid value of 'driving side'"), | |
| std::string("Valid value are 'r', 'l', 'b'")); | |
| } | |
| break; | |
| } | |
| default: { | |
| /* For the moment its old signature of pgr_withPoints */ | |
| if (!((d_side == 'r') || (d_side == 'l'))) d_side = 'b'; | |
| } | |
| } | |
| return d_side; | |
| } |
🧰 Tools
🪛 Cppcheck (2.19.0)
[style] 88-88: The function 'estimate_drivingSide' is never used.
(unusedFunction)
🪛 GitHub Actions: Check files
[error] 97-97: cpplint: Add #include for make_pair. [build/include_what_you_use]
🤖 Prompt for AI Agents
In `@src/cpp_common/utilities.cpp` around lines 87 - 108, In estimate_drivingSide,
avoid undefined behavior by calling tolower with an unsigned char cast: replace
tolower(driving_side) with tolower(static_cast<unsigned char>(driving_side));
also remove the unreachable "break;" after the "return ' ';" in the DIJKSTRA
case; ensure the function still handles WHICH cases DIJKSTRA, WITHPOINTS and
default as before (no other logic changes).
| switch (which) { | ||
| case BANDWIDTH: { | ||
| result = pgrouting::metrics::bandwidth(undigraph); | ||
| break; | ||
| } | ||
| default: | ||
| break; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a defensive log for unexpected enum values.
The switch-case structure is extensible for future metrics. However, the default case silently returns 0, which could mask programming errors if an invalid or unhandled Which value is passed. Consider logging a warning or adding a comment to clarify this is intentional.
♻️ Optional: Add defensive logging for unexpected values
switch (which) {
case BANDWIDTH: {
result = pgrouting::metrics::bandwidth(undigraph);
break;
}
default:
+ log << "Unexpected metrics type";
break;
}🧰 Tools
🪛 Cppcheck (2.19.0)
[performance] 81-81: Function parameter 'data_edges' should be passed by const reference.
(passedByValue)
🤖 Prompt for AI Agents
In `@src/metrics/metrics_driver.cpp` around lines 76 - 83, The switch in
metrics_driver.cpp handling the Which enum currently returns 0 silently in the
default branch; update the default case in the function containing the switch
(the branch handling BANDWIDTH -> pgrouting::metrics::bandwidth(undigraph)) to
emit a defensive warning or error log that includes the unexpected Which value
(or call site) before returning, or alternatively add an assertion/exception if
unhandled values are programming errors, so unexpected enum values are visible
in logs.
- Also a function used in shortestPath driver
cvvergara
force-pushed
the
using-enumeration-on-existing-drivers
branch
from
de46bcd to
e31721a
Compare
2 participants
Fixes #3049
Changes proposed in this pull request:
@pgRouting/admins
Summary by CodeRabbit
Release Notes
Code Enhancements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.