Components: use process & drivers ordering and coloring#3114
Merged
cvvergara merged 17 commits intoJun 10, 2026
Merged
Conversation
- Add directed parameter to handle directed and undirected functions - Prepare to process applicable component functions
- cleanup code - standardize code
- Add directed parameter to handle directed and undirected functions - Prepare to process applicable component functions
- cleanup code - standardize code
Contributor
WalkthroughRoute component algorithms through shared coloring/ordering process and driver functions, extend ChangesComponent algorithms integrate into process/driver pairs
Sequence Diagram(s)(no sequence diagrams generated) 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 |
robe2
approved these changes
Jun 10, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/process/ordering_process.h (1)
36-39:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing
stdbool.hinclude for C compilation.The function signature at line 47 uses
bool, but the C block (lines 36-39) does not includestdbool.h. This will cause a compilation error when this header is included from C code sinceboolis not a built-in type in C.Compare with
coloring_process.hwhich correctly includesstdbool.hat line 40.🐛 Proposed fix
`#else` `#include` <stddef.h> `#include` <stdint.h> +#include <stdbool.h> `#endif`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/process/ordering_process.h` around lines 36 - 39, The C compilation branch in ordering_process.h is missing `#include` <stdbool.h>, causing the use of the bool type in the function signature to fail; add an `#include` <stdbool.h> to the C-side include block (alongside the existing <stddef.h> and <stdint.h>) so the bool type used by the function signature in ordering_process.h is defined for C builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@include/cpp_common/to_postgres.hpp`:
- Around line 86-87: The declaration of get_tuples currently takes the nested
vector by value causing an expensive copy; change its signature to accept a
const reference: use const std::vector<std::vector<int64_t>>& for the first
parameter in the get_tuples declaration and update any corresponding
definition(s) (and all callers) to match, ensuring no mutable operations are
attempted on that parameter (or make a local copy where mutation is required).
In `@src/coloring/coloring_driver.cpp`:
- Around line 113-116: The error message in the default case of the
directed-graph branch incorrectly says "for undirected graph"; update the
message to say "for directed graph" in coloring_driver.cpp (and mirror the same
fix in ordering_driver.cpp) by editing the default branch that builds the error
string (the code that uses get_name(which) and writes to err) so the final text
reads "... for directed graph" instead of "... for undirected graph".
In `@src/components/bridges.c`:
- Line 74: The local variable call_cntr in the SRF implementation downcasts
funcctx->call_cntr (a uint64_t) to uint32_t causing possible truncation and
inconsistency with other SRFs like articulationPoints.c; change the declaration
of call_cntr to uint64_t and use funcctx->call_cntr directly, and when returning
the value (the datum produced around the current output path) switch to
Int64GetDatum if the output column expects a 64-bit integer (replace any
UInt32GetDatum/Int32GetDatum usage accordingly).
In `@src/components/components.cpp`:
- Around line 72-76: The typedef in strongComponents is using the wrong graph
type: change the vertex descriptor typedef from pgrouting::UndirectedGraph::V to
the correct directed graph type (pgrouting::DirectedGraph::V) so the function
strongComponents uses the proper vertex descriptor for the DirectedGraph
parameter; update the typedef declaration to reference
pgrouting::DirectedGraph::V (or use graph::V if that alias exists) and rebuild
to ensure type consistency.
In `@src/components/makeConnected.cpp`:
- Line 2: Update the top-of-file header comment in makeConnected.cpp so the
"File:" line correctly names makeConnected.cpp instead of makeConnected.hpp;
locate the header comment near the top of the source (in the makeConnected.cpp
file) and change the filename token to ".cpp" to match the actual source file.
- Around line 82-90: The try-catch wrapper that catches boost::exception,
std::exception, and (...) only suppresses the exception variables with (void)
and immediately rethrows, which is redundant; remove the entire try { ... }
catch(...) { ... } wrapper in makeConnected.cpp (the blocks catching
boost::exception, std::exception&, and ...) so the original code inside the try
can let exceptions propagate naturally, and ensure you also remove the matching
try and closing brace so there are no unmatched braces or scope changes.
In `@src/cpp_common/to_postgres.cpp`:
- Around line 247-267: The function get_tuples uses unqualified
sort(components.begin(), components.end()) which relies on ADL; change this to
std::sort(components.begin(), components.end()) for consistency with the earlier
std::sort usage and to avoid ambiguity; update the call in get_tuples that sorts
the outer vector (components) to use the std:: prefix while leaving the
inner-component sorts intact.
---
Outside diff comments:
In `@include/process/ordering_process.h`:
- Around line 36-39: The C compilation branch in ordering_process.h is missing
`#include` <stdbool.h>, causing the use of the bool type in the function signature
to fail; add an `#include` <stdbool.h> to the C-side include block (alongside the
existing <stddef.h> and <stdint.h>) so the bool type used by the function
signature in ordering_process.h is defined for C builds.
🪄 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: 8e611969-b9b9-47a7-acb1-0e6cfb33529a
📒 Files selected for processing (48)
NEWS.mddoc/src/release_notes.rstinclude/c_common/enums.hinclude/components/components.hppinclude/components/componentsResult.hppinclude/components/makeConnected.hppinclude/cpp_common/to_postgres.hppinclude/drivers/coloring_driver.hppinclude/drivers/components/articulationPoints_driver.hinclude/drivers/components/biconnectedComponents_driver.hinclude/drivers/components/bridges_driver.hinclude/drivers/components/connectedComponents_driver.hinclude/drivers/components/makeConnected_driver.hinclude/drivers/components/strongComponents_driver.hinclude/drivers/ordering_driver.hppinclude/process/coloring_process.hinclude/process/ordering_process.hlocale/en/LC_MESSAGES/pgrouting_doc_strings.polocale/pot/pgrouting_doc_strings.potsrc/coloring/bipartite.csrc/coloring/coloring_driver.cppsrc/coloring/coloring_process.cppsrc/coloring/edgeColoring.csrc/coloring/sequentialVertexColoring.csrc/components/CMakeLists.txtsrc/components/articulationPoints.csrc/components/articulationPoints_driver.cppsrc/components/biconnectedComponents.csrc/components/biconnectedComponents_driver.cppsrc/components/bridges.csrc/components/bridges_driver.cppsrc/components/components.cppsrc/components/componentsResult.cppsrc/components/connectedComponents.csrc/components/connectedComponents_driver.cppsrc/components/makeConnected.csrc/components/makeConnected.cppsrc/components/makeConnected_driver.cppsrc/components/strongComponents.csrc/components/strongComponents_driver.cppsrc/cpp_common/to_postgres.cppsrc/cpp_common/utilities.cppsrc/ordering/cuthillMckeeOrdering.csrc/ordering/kingOrdering.csrc/ordering/ordering_driver.cppsrc/ordering/ordering_process.cppsrc/ordering/sloanOrdering.csrc/ordering/topologicalSort.c
💤 Files with no reviewable changes (14)
- include/drivers/components/makeConnected_driver.h
- include/components/componentsResult.hpp
- src/components/biconnectedComponents_driver.cpp
- include/drivers/components/connectedComponents_driver.h
- include/drivers/components/articulationPoints_driver.h
- src/components/strongComponents_driver.cpp
- src/components/articulationPoints_driver.cpp
- include/drivers/components/bridges_driver.h
- include/drivers/components/strongComponents_driver.h
- src/components/componentsResult.cpp
- include/drivers/components/biconnectedComponents_driver.h
- src/components/bridges_driver.cpp
- src/components/makeConnected_driver.cpp
- src/components/connectedComponents_driver.cpp
Comment on lines
+113
to
+116
| default: | ||
| err << __FILE_NAME__ << ": Unknown function with name '" << get_name(which) | ||
| << "' for directed graph"; | ||
| return; |
Contributor
There was a problem hiding this comment.
Copy-paste error: "undirected" in directed branches across both drivers.
Both coloring_driver.cpp and ordering_driver.cpp have the same bug where the error message in the directed == true branch incorrectly states "for undirected graph". This appears to be a copy-paste error when implementing the directed/undirected branching logic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/coloring/coloring_driver.cpp` around lines 113 - 116, The error message
in the default case of the directed-graph branch incorrectly says "for
undirected graph"; update the message to say "for directed graph" in
coloring_driver.cpp (and mirror the same fix in ordering_driver.cpp) by editing
the default branch that builds the error string (the code that uses
get_name(which) and writes to err) so the final text reads "... for directed
graph" instead of "... for undirected graph".
Contributor
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/ordering/ordering_driver.cpp (1)
63-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset the output parameters before any early return path.
return_countis only assigned inside the non-empty result branches, but Line 178 reads it unconditionally. When an algorithm legitimately returns no rows—or this function exits earlier on an error/empty-input path—the result depends on whatever state the caller happened to pass in, which can skip the"No results found"branch or leak stale tuples downstream.Suggested fix
void do_ordering( const std::string &edges_sql, bool directed, Which which, int64_t *&return_tuples, size_t &return_count, std::ostringstream &log, std::ostringstream ¬ice, std::ostringstream &err) { + return_tuples = nullptr; + return_count = 0; std::string hint = ""; try {Also applies to: 170-180
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ordering/ordering_driver.cpp` around lines 63 - 64, The function leaves output params return_tuples and return_count uninitialized on early/empty-return paths; ensure you initialize and reset them (e.g., set return_tuples to nullptr and return_count to 0) at the start of the function and before any early return paths (including error/empty-input branches and the sections around lines 170-180) so callers never observe stale values; update all early-return spots to explicitly set these output parameters before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/ordering/ordering_driver.cpp`:
- Around line 63-64: The function leaves output params return_tuples and
return_count uninitialized on early/empty-return paths; ensure you initialize
and reset them (e.g., set return_tuples to nullptr and return_count to 0) at the
start of the function and before any early return paths (including
error/empty-input branches and the sections around lines 170-180) so callers
never observe stale values; update all early-return spots to explicitly set
these output parameters before returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c065f1e2-2969-4b67-98f7-ab2b2d0f5b48
📒 Files selected for processing (1)
src/ordering/ordering_driver.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3113 .
Summary by CodeRabbit
New Features
Refactor