coloring using one process and driver#3103
Open
cvvergara wants to merge 13 commits into
Open
Conversation
Contributor
WalkthroughThis PR centralizes coloring processing into a unified C/C++ process and driver, converts class/template algorithm implementations to free functions on pgrouting::UndirectedGraph, updates SRF C wrappers to call the process, adjusts build files, tests, docs, and locales, and extends the dispatch enum. ChangesColoring refactor
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/update.yml:
- Line 50: The workflow currently sets the GitHub Actions matrix option
fail-fast: true which cancels remaining matrix jobs (including old_pgr entries)
after the first failure; change fail-fast to false (or remove the key) in the
workflow definition so the matrix runs all entries and preserves full signal for
old_pgr jobs—update the fail-fast setting in the job/matrix block where
fail-fast: true is declared.
In `@include/coloring/edgeColoring.hpp`:
- Line 42: The declaration of edgeColoring currently takes
pgrouting::UndirectedGraph by value which causes unnecessary copies; change the
function signature and all matching definitions/overloads to accept a const
reference (const pgrouting::UndirectedGraph& graph) and update any callers to
pass the graph by reference; ensure the header declaration (edgeColoring) and
its implementation use the same const-reference type and rebuild to catch any
mismatched signatures.
In `@include/coloring/sequentialVertexColoring.hpp`:
- Line 40: The declaration of sequentialVertexColoring has inconsistent leading
whitespace; update the function prototype for sequentialVertexColoring(const
pgrouting::UndirectedGraph&) so its indentation matches surrounding declarations
(remove the extra leading spaces before "std::vector<II_t_rt>
sequentialVertexColoring(...)" to align with the file's style).
In `@src/coloring/bipartite.cpp`:
- Line 78: Change the pgr_bipartite function signature to take the graph by
const reference since it is not modified: update the implementation signature
std::vector<II_t_rt> pgr_bipartite(const pgrouting::UndirectedGraph& graph) and
also update the corresponding declaration for pgr_bipartite in the header to
match; ensure any callers still compile (they should) and adjust any non-const
contexts if necessary.
- Around line 58-60: The thrown error message incorrectly references
"boost::edge_coloring" even though this is the bipartite implementation; update
the string passed in the throw (the std::make_pair in bipartite.cpp) to
reference the bipartite implementation (e.g., "INTERNAL: something went wrong
while calling bipartite edge coloring" or similar) so the error accurately names
the failing component while leaving the second element (__PGR_PRETTY_FUNCTION__)
unchanged.
- Around line 78-80: The code currently calls boost::is_bipartite twice (once in
pgr_bipartite and again inside get_bipartition), causing redundant graph
traversal; modify pgr_bipartite and get_bipartition so the bipartiteness result
is computed once and reused—either by changing get_bipartition to accept a
precomputed bool (e.g., add a parameter like bool is_bipartite) or by moving the
partition extraction logic into pgr_bipartite and passing the graph and the
single is_bipartite result to the partition routine; ensure you remove the
internal boost::is_bipartite call from get_bipartition and use the passed flag
to decide whether to return an empty vector or the partition.
In `@src/coloring/coloring_driver.cpp`:
- Around line 103-105: The default error branch concatenates the message and
function name without a separator; update the default case so the err stream
includes a separator before calling get_name(which) (e.g., add a space or ": ")
so the output becomes "Unknown coloring function: EDGECOLORING" when locating
the code around the default branch that writes to err and calls get_name(which).
In `@src/coloring/coloring_process.cpp`:
- Around line 55-56: The using-declaration imports pgr_free but the code calls
pfree directly (see symbols pgr_free and pfree); remove the unused
using-declaration for pgr_free from the top of coloring_process.cpp (keep
to_pg_msg if still used) so there is no dead import, or alternatively replace
calls to pfree with pgr_free if pgr_free is the intended wrapper—ensure
consistency by choosing one symbol (pgr_free or pfree) and updating the using
list and call sites accordingly.
- Around line 57-68: The code asserts edges_sql is non-null via
pgassert(edges_sql) but then uses a defensive ternary edges_sql ? edges_sql : ""
when calling pgrouting::drivers::do_coloring; remove the redundant ternary and
pass edges_sql directly to do_coloring (or conversely remove the pgassert and
keep the ternary if you prefer null-safe behavior) so the checks are
consistent—update the call site in the do_coloring invocation to use edges_sql
and ensure only one null-handling strategy is present.
In `@src/coloring/edgeColoring.c`:
- Around line 3-5: Remove the duplicated header comment "Generated with Template
by:" in src/coloring/edgeColoring.c so it appears only once; edit the
top-of-file comment block (the header around the existing "Generated with
Template by:" lines) to eliminate the redundant line and preserve any
surrounding header text or metadata.
In `@src/coloring/edgeColoring.cpp`:
- Around line 80-85: Fix the comment typo in edgeColoring.cpp: change
"outsatnding" to "outstanding" in the block above the assignment that sets
results[i].d2.value (the comment that references boost and color_map/get). Leave
the code statement (results[i].d2.value = ...) unchanged.
- Around line 48-49: The edgeColoring function currently takes the graph
parameter by value (edgeColoring(pgrouting::UndirectedGraph g)), causing an
unnecessary copy; change the signature to accept a const reference
(edgeColoring(const pgrouting::UndirectedGraph& g)) and update any callers if
needed, ensuring you only read from g inside edgeColoring (or explicitly copy
where mutation is required) and keep references to member functions or iterators
compatible with a const graph.
In `@src/coloring/sequentialVertexColoring.cpp`:
- Around line 104-114: The catch-all handler after calling
boost::sequential_vertex_coloring incorrectly mentions "boost::edge_coloring" in
the thrown error; update the error message text in the throw inside the
catch(...) to reference "boost::sequential_vertex_coloring" (and ensure the
thrown pair still contains the descriptive message plus
__PGR_PRETTY_FUNCTION__), and remove any stray punctuation (extra semicolon) if
present so the exception text correctly identifies the failing 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: ef849f12-4b0b-4875-8445-cc31c475d5e9
📒 Files selected for processing (28)
.github/workflows/update.ymlNEWS.mddoc/coloring/pgr_edgeColoring.rstdoc/src/release_notes.rstdocqueries/coloring/edgeColoring.resultinclude/c_common/enums.hinclude/coloring/bipartite.hppinclude/coloring/bipartite_driver.hppinclude/coloring/edgeColoring.hppinclude/coloring/sequentialVertexColoring.hppinclude/drivers/coloring_driver.hppinclude/process/coloring_process.hlocale/en/LC_MESSAGES/pgrouting_doc_strings.polocale/pot/pgrouting_doc_strings.potpgtap/coloring/edgeColoring/edge_cases.pgsrc/coloring/CMakeLists.txtsrc/coloring/bipartite.csrc/coloring/bipartite.cppsrc/coloring/bipartite_driver.cppsrc/coloring/coloring_driver.cppsrc/coloring/coloring_process.cppsrc/coloring/edgeColoring.csrc/coloring/edgeColoring.cppsrc/coloring/edgeColoring_driver.cppsrc/coloring/sequentialVertexColoring.csrc/coloring/sequentialVertexColoring.cppsrc/coloring/sequentialVertexColoring_driver.cpptools/scripts/code_checker.sh
💤 Files with no reviewable changes (4)
- src/coloring/sequentialVertexColoring_driver.cpp
- src/coloring/bipartite_driver.cpp
- src/coloring/edgeColoring_driver.cpp
- include/coloring/bipartite_driver.hpp
| int64_t get_vertex_id(V v) const; | ||
| int64_t get_edge_id(E e) const; | ||
|
|
||
| std::vector<II_t_rt> edgeColoring(pgrouting::UndirectedGraph); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the edgeColoring implementation to see if the graph is modified
rg -n -A 30 'edgeColoring\s*\(' src/coloring/edgeColoring.cpp | head -50Repository: pgRouting/pgrouting
Length of output: 1278
Consider passing UndirectedGraph by const-reference instead of by value.
The implementation reads from the graph but does not modify it. Passing by const& avoids unnecessary copying for large graphs:
Suggested change
-std::vector<II_t_rt> edgeColoring(pgrouting::UndirectedGraph);
+std::vector<II_t_rt> edgeColoring(const pgrouting::UndirectedGraph&);📝 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.
Suggested change
| std::vector<II_t_rt> edgeColoring(pgrouting::UndirectedGraph); | |
| std::vector<II_t_rt> edgeColoring(const pgrouting::UndirectedGraph&); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/coloring/edgeColoring.hpp` at line 42, The declaration of
edgeColoring currently takes pgrouting::UndirectedGraph by value which causes
unnecessary copies; change the function signature and all matching
definitions/overloads to accept a const reference (const
pgrouting::UndirectedGraph& graph) and update any callers to pass the graph by
reference; ensure the header declaration (edgeColoring) and its implementation
use the same const-reference type and rebuild to catch any mismatched
signatures.
Comment on lines
+58
to
+60
| throw std::make_pair( | ||
| std::string("INTERNAL: something went wrong while calling boost::edge_coloring"), | ||
| std::string(__PGR_PRETTY_FUNCTION__));; |
Contributor
There was a problem hiding this comment.
Incorrect error message references wrong algorithm.
The catch block mentions "boost::edge_coloring" but this is the bipartite implementation.
✏️ Suggested fix
throw std::make_pair(
- std::string("INTERNAL: something went wrong while calling boost::edge_coloring"),
+ std::string("INTERNAL: something went wrong while calling boost::is_bipartite"),
std::string(__PGR_PRETTY_FUNCTION__));;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coloring/bipartite.cpp` around lines 58 - 60, The thrown error message
incorrectly references "boost::edge_coloring" even though this is the bipartite
implementation; update the string passed in the throw (the std::make_pair in
bipartite.cpp) to reference the bipartite implementation (e.g., "INTERNAL:
something went wrong while calling bipartite edge coloring" or similar) so the
error accurately names the failing component while leaving the second element
(__PGR_PRETTY_FUNCTION__) unchanged.
| namespace pgrouting { | ||
| namespace functions { | ||
|
|
||
| std::vector<II_t_rt> pgr_bipartite(pgrouting::UndirectedGraph &graph ) { |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Parameter could be const reference.
The graph parameter is not modified; consider passing by const reference for clarity.
♻️ Suggested fix
-std::vector<II_t_rt> pgr_bipartite(pgrouting::UndirectedGraph &graph ) {
+std::vector<II_t_rt> pgr_bipartite(const pgrouting::UndirectedGraph &graph) {Note: This requires updating the header declaration in include/coloring/bipartite.hpp to match.
📝 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.
Suggested change
| std::vector<II_t_rt> pgr_bipartite(pgrouting::UndirectedGraph &graph ) { | |
| std::vector<II_t_rt> pgr_bipartite(const pgrouting::UndirectedGraph &graph) { |
🧰 Tools
🪛 Cppcheck (2.20.0)
[style] 78-78: The function 'pgr_bipartite' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coloring/bipartite.cpp` at line 78, Change the pgr_bipartite function
signature to take the graph by const reference since it is not modified: update
the implementation signature std::vector<II_t_rt> pgr_bipartite(const
pgrouting::UndirectedGraph& graph) and also update the corresponding declaration
for pgr_bipartite in the header to match; ensure any callers still compile (they
should) and adjust any non-const contexts if necessary.
Comment on lines
+57
to
+68
| pgassert(edges_sql); | ||
| pgassert(!(*result_tuples)); | ||
| pgassert(*result_count == 0); | ||
| pgr_SPI_connect(); | ||
|
|
||
| std::ostringstream log; | ||
| std::ostringstream err; | ||
| std::ostringstream notice; | ||
|
|
||
| clock_t start_t = clock(); | ||
| pgrouting::drivers::do_coloring( | ||
| edges_sql? edges_sql : "", |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor inconsistency: assertion vs defensive null check.
Line 57 asserts edges_sql is non-null, but line 68 defensively handles null with edges_sql? edges_sql : "". The assertion makes the ternary unreachable in debug builds. Consider either:
- Removing the assertion if null should be gracefully handled, or
- Removing the ternary if null is truly a programming error
♻️ Option A: Trust the assertion (remove ternary)
pgrouting::drivers::do_coloring(
- edges_sql? edges_sql : "",
+ edges_sql,♻️ Option B: Allow null (remove assertion)
- pgassert(edges_sql);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coloring/coloring_process.cpp` around lines 57 - 68, The code asserts
edges_sql is non-null via pgassert(edges_sql) but then uses a defensive ternary
edges_sql ? edges_sql : "" when calling pgrouting::drivers::do_coloring; remove
the redundant ternary and pass edges_sql directly to do_coloring (or conversely
remove the pgassert and keep the ternary if you prefer null-safe behavior) so
the checks are consistent—update the call site in the do_coloring invocation to
use edges_sql and ensure only one null-handling strategy is present.
Comment on lines
3
to
+5
| Generated with Template by: | ||
|
|
||
| Generated with Template by: |
Contributor
There was a problem hiding this comment.
Minor: Duplicate comment line.
"Generated with Template by:" appears twice in the header (lines 3 and 5).
✏️ Suggested fix
/*PGR-GNU*****************************************************************
File: edgeColoring.c
-Generated with Template by:
Generated with Template by:
Copyright (c) 2015-2026 pgRouting developers📝 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.
Suggested change
| Generated with Template by: | |
| Generated with Template by: | |
| /*PGR-GNU***************************************************************** | |
| File: edgeColoring.c | |
| Generated with Template by: | |
| Copyright (c) 2015-2026 pgRouting developers |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coloring/edgeColoring.c` around lines 3 - 5, Remove the duplicated header
comment "Generated with Template by:" in src/coloring/edgeColoring.c so it
appears only once; edit the top-of-file comment block (the header around the
existing "Generated with Template by:" lines) to eliminate the redundant line
and preserve any surrounding header text or metadata.
Comment on lines
+80
to
+85
| /** | ||
| * There is a problem with boost: | ||
| * Sometimes it returns a color with outsatnding large value | ||
| * When that happens changing color to: colors + 1 | ||
| */ | ||
| results[i].d2.value = get(color_map, *ei) < colors? get(color_map, *ei) + 1 : colors + 1; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Typo in comment.
"outsatnding" should be "outstanding".
Proposed fix
/**
* There is a problem with boost:
- * Sometimes it returns a color with outsatnding large value
+ * Sometimes it returns a color with outstanding large value
* When that happens changing color to: colors + 1
*/📝 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.
Suggested change
| /** | |
| * There is a problem with boost: | |
| * Sometimes it returns a color with outsatnding large value | |
| * When that happens changing color to: colors + 1 | |
| */ | |
| results[i].d2.value = get(color_map, *ei) < colors? get(color_map, *ei) + 1 : colors + 1; | |
| /** | |
| * There is a problem with boost: | |
| * Sometimes it returns a color with outstanding large value | |
| * When that happens changing color to: colors + 1 | |
| */ | |
| results[i].d2.value = get(color_map, *ei) < colors? get(color_map, *ei) + 1 : colors + 1; |
🧰 Tools
🪛 Cppcheck (2.20.0)
[performance] 81-81: Function parameter 'data_edges' should be passed by const reference.
(passedByValue)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coloring/edgeColoring.cpp` around lines 80 - 85, Fix the comment typo in
edgeColoring.cpp: change "outsatnding" to "outstanding" in the block above the
assignment that sets results[i].d2.value (the comment that references boost and
color_map/get). Leave the code statement (results[i].d2.value = ...) unchanged.
Comment on lines
+104
to
+114
| try { | ||
| boost::sequential_vertex_coloring(graph.graph, color_map); | ||
| } catch (boost::exception const& ex) { | ||
| throw; | ||
| } catch (std::exception &e) { | ||
| throw; | ||
| } catch (...) { | ||
| throw std::make_pair( | ||
| std::string("INTERNAL: something went wrong while calling boost::edge_coloring"), | ||
| std::string(__PGR_PRETTY_FUNCTION__));; | ||
| } |
Contributor
There was a problem hiding this comment.
Incorrect algorithm name in error message.
The error message references boost::edge_coloring but this function calls boost::sequential_vertex_coloring.
Proposed fix
} catch (...) {
throw std::make_pair(
- std::string("INTERNAL: something went wrong while calling boost::edge_coloring"),
+ std::string("INTERNAL: something went wrong while calling boost::sequential_vertex_coloring"),
std::string(__PGR_PRETTY_FUNCTION__));;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coloring/sequentialVertexColoring.cpp` around lines 104 - 114, The
catch-all handler after calling boost::sequential_vertex_coloring incorrectly
mentions "boost::edge_coloring" in the thrown error; update the error message
text in the throw inside the catch(...) to reference
"boost::sequential_vertex_coloring" (and ensure the thrown pair still contains
the descriptive message plus __PGR_PRETTY_FUNCTION__), and remove any stray
punctuation (extra semicolon) if present so the exception text correctly
identifies the failing function.
robe2
approved these changes
Mar 31, 2026
Member
robe2
left a comment
robe2
left a comment
There was a problem hiding this comment.
Make sure to review coderabbitai major issues
cvvergara
changed the title
cvvergara
force-pushed
the
coloring-using-one-process_and-driver
branch
from
ad92dc2 to
b561cd0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@doc/src/release_notes.rst`:
- Line 52: Fix the typo in the release note entry "* :issue:`3100`: Coloring:
create an use a process & driver" by changing "create an use" to "create and
use" so the line reads "* :issue:`3100`: Coloring: create and use a process &
driver" (update the string in doc/src/release_notes.rst where the issue 3100
entry appears).
In `@locale/en/LC_MESSAGES/pgrouting_doc_strings.po`:
- Around line 3974-3977: Update the msgid string that reads "`#3100
<https://github.com/pgRouting/pgrouting/issues/3100>`__: Coloring: create an use
a process & driver" by changing "an use" to "and use"; specifically edit the
msgid in the PO entry (the quoted string starting with "`#3100 ... Coloring:
create an use a process & driver") to read "... Coloring: create and use a
process & driver" and, if applicable, adjust the corresponding msgstr to the
corrected English text or provide the proper translation.
In `@locale/pot/pgrouting_doc_strings.pot`:
- Line 3559: Update the message string with the typo fix: locate the msgid text
"`#3100 <https://github.com/pgRouting/pgrouting/issues/3100>`__: Coloring:
create an use a process & driver" and change "create an use" to "create and use"
so the entry reads "Coloring: create and use a process & driver".
In `@NEWS.md`:
- Line 27: Update the release note text for PR `#3100` that currently reads
"create an use a process & driver" to the corrected phrase "create and use a
process & driver" by replacing "an" with "and" in that NEWS.md entry.
In `@pgtap/coloring/edgeColoring/edge_cases.pg`:
- Around line 110-116: The VALUES fixtures feeding pgr_edgeColoring are
unordered and can produce flaky results; update each PREPARE statement (e.g.,
PREPARE q8, q9, q10, q11, q12, q15) that selects FROM (VALUES ...) AS t(...) to
wrap the VALUES subquery with an explicit ORDER BY id ASC so the edge rows are
deterministically ordered before calling pgr_edgeColoring; ensure every
VALUES-based query in this file using ids (the same pattern around
q8/q9/q10/q11/q12/q15) gets the ORDER BY id ASC applied.
- Around line 258-259: The q15 test only asserts count(DISTINCT color)=1 but not
that both disconnected edges are returned; update the single TAP assertion for
q15 to validate both output cardinality and color uniqueness in one check
against pgr_edgeColoring('q15') — e.g., replace the current distinct-color-only
assertion with a single assertion that ensures the result set has two rows (both
edges returned) and that count(DISTINCT color)=1, referencing
pgr_edgeColoring('q15') and keeping it as one TAP is() assertion.
In `@src/coloring/bipartite.cpp`:
- Around line 46-48: Replace the wrapper call graph.num_vertices() with the
direct BGL function boost::num_vertices(graph.graph) when sizing the partition
vector and creating partition_map; update the allocation of
std::vector<boost::default_color_type> partition(...) to use
boost::num_vertices(graph.graph) and ensure make_iterator_property_map still
uses partition.begin() with boost::get(boost::vertex_index, graph.graph) so the
code references the underlying Boost graph directly (symbols: partition,
partition_map, graph.num_vertices(), boost::num_vertices(graph.graph)).
In `@src/coloring/coloring_driver.cpp`:
- Around line 52-61: At the top of do_coloring initialize the output parameters
to safe defaults so callers never see stale values: set return_tuples to null
(no allocated pointer) and return_count to 0 before any branching/early returns,
and ensure any early-return paths leave those defaults intact; update all
branches that currently return early (including the error/notice paths
referenced around the shown ranges) to rely on these initialized values rather
than assuming they were set later.
🪄 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: 227e211a-8824-41d5-90fa-da7a13a67b9d
📒 Files selected for processing (28)
.github/workflows/update.ymlNEWS.mddoc/coloring/pgr_edgeColoring.rstdoc/src/release_notes.rstdocqueries/coloring/edgeColoring.resultinclude/c_common/enums.hinclude/coloring/bipartite.hppinclude/coloring/bipartite_driver.hppinclude/coloring/edgeColoring.hppinclude/coloring/sequentialVertexColoring.hppinclude/drivers/coloring_driver.hppinclude/process/coloring_process.hlocale/en/LC_MESSAGES/pgrouting_doc_strings.polocale/pot/pgrouting_doc_strings.potpgtap/coloring/edgeColoring/edge_cases.pgsrc/coloring/CMakeLists.txtsrc/coloring/bipartite.csrc/coloring/bipartite.cppsrc/coloring/bipartite_driver.cppsrc/coloring/coloring_driver.cppsrc/coloring/coloring_process.cppsrc/coloring/edgeColoring.csrc/coloring/edgeColoring.cppsrc/coloring/edgeColoring_driver.cppsrc/coloring/sequentialVertexColoring.csrc/coloring/sequentialVertexColoring.cppsrc/coloring/sequentialVertexColoring_driver.cpptools/scripts/code_checker.sh
💤 Files with no reviewable changes (4)
- src/coloring/sequentialVertexColoring_driver.cpp
- include/coloring/bipartite_driver.hpp
- src/coloring/bipartite_driver.cpp
- src/coloring/edgeColoring_driver.cpp
| * :issue:`3086`: MaxFlow: create and use a process and driver | ||
| * :issue:`3089`: edgeDisjoint and bellmanFord use shortestPath driver and | ||
| process | ||
| * :issue:`3100`: Coloring: create an use a process & driver |
Contributor
There was a problem hiding this comment.
Fix typo in release-note entry.
Line 52 says “create an use”; this should be “create and use”.
✏️ Proposed fix
-* :issue:`3100`: Coloring: create an use a process & driver
+* :issue:`3100`: Coloring: create and use a process & driver📝 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.
Suggested change
| * :issue:`3100`: Coloring: create an use a process & driver | |
| * :issue:`3100`: Coloring: create and use a process & driver |
🤖 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 `@doc/src/release_notes.rst` at line 52, Fix the typo in the release note entry
"* :issue:`3100`: Coloring: create an use a process & driver" by changing
"create an use" to "create and use" so the line reads "* :issue:`3100`:
Coloring: create and use a process & driver" (update the string in
doc/src/release_notes.rst where the issue 3100 entry appears).
Comment on lines
+3974
to
+3977
| msgid "" | ||
| "`#3100 <https://github.com/pgRouting/pgrouting/issues/3100>`__: Coloring: " | ||
| "create an use a process & driver" | ||
| msgstr "" |
Contributor
There was a problem hiding this comment.
Fix typo in release note text ("an use" → "and use").
This is user-facing release-note text and currently reads awkwardly.
✏️ Proposed fix
msgid ""
"`#3100 <https://github.com/pgRouting/pgrouting/issues/3100>`__: Coloring: "
-"create an use a process & driver"
+"create and use a process & driver"
msgstr ""📝 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.
Suggested change
| msgid "" | |
| "`#3100 <https://github.com/pgRouting/pgrouting/issues/3100>`__: Coloring: " | |
| "create an use a process & driver" | |
| msgstr "" | |
| msgid "" | |
| "`#3100 <https://github.com/pgRouting/pgrouting/issues/3100>`__: Coloring: " | |
| "create and use a process & driver" | |
| msgstr "" |
🤖 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 `@locale/en/LC_MESSAGES/pgrouting_doc_strings.po` around lines 3974 - 3977,
Update the msgid string that reads "`#3100
<https://github.com/pgRouting/pgrouting/issues/3100>`__: Coloring: create an use
a process & driver" by changing "an use" to "and use"; specifically edit the
msgid in the PO entry (the quoted string starting with "`#3100 ... Coloring:
create an use a process & driver") to read "... Coloring: create and use a
process & driver" and, if applicable, adjust the corresponding msgstr to the
corrected English text or provide the proper translation.
| msgid "`#3089 <https://github.com/pgRouting/pgrouting/issues/3089>`__: edgeDisjoint and bellmanFord use shortestPath driver and process" | ||
| msgstr "" | ||
|
|
||
| msgid "`#3100 <https://github.com/pgRouting/pgrouting/issues/3100>`__: Coloring: create an use a process & driver" |
Contributor
There was a problem hiding this comment.
Fix typo in release note entry for issue #3100.
Line 3559 has a grammar typo: “create an use” should be “create and use”.
🤖 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 `@locale/pot/pgrouting_doc_strings.pot` at line 3559, Update the message string
with the typo fix: locate the msgid text "`#3100
<https://github.com/pgRouting/pgrouting/issues/3100>`__: Coloring: create an use
a process & driver" and change "create an use" to "create and use" so the entry
reads "Coloring: create and use a process & driver".
| * [#3086](https://github.com/pgRouting/pgrouting/issues/3086): MaxFlow: create and use a process and driver | ||
| * [#3089](https://github.com/pgRouting/pgrouting/issues/3089): edgeDisjoint and bellmanFord use shortestPath driver and | ||
| process | ||
| * [#3100](https://github.com/pgRouting/pgrouting/issues/3100): Coloring: create an use a process & driver |
Contributor
There was a problem hiding this comment.
Fix typo in release-note text.
Line 27 says “create an use”; this should be “create and use”.
✏️ Proposed fix
-* [`#3100`](https://github.com/pgRouting/pgrouting/issues/3100): Coloring: create an use a process & driver
+* [`#3100`](https://github.com/pgRouting/pgrouting/issues/3100): Coloring: create and use a process & driver📝 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.
Suggested change
| * [#3100](https://github.com/pgRouting/pgrouting/issues/3100): Coloring: create an use a process & driver | |
| * [`#3100`](https://github.com/pgRouting/pgrouting/issues/3100): Coloring: create and use a process & driver |
🤖 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 `@NEWS.md` at line 27, Update the release note text for PR `#3100` that currently
reads "create an use a process & driver" to the corrected phrase "create and use
a process & driver" by replacing "an" with "and" in that NEWS.md entry.
Comment on lines
110
to
+116
| PREPARE q8 AS | ||
| SELECT id, source, target, cost, reverse_cost | ||
| FROM three_vertices_table; | ||
| FROM (VALUES | ||
| (1, 3, 6, 20, 15), | ||
| (2, 3, 8, 10, -10), | ||
| (3, 6, 8, -1, 12)) | ||
| AS t(id, source, target, cost, reverse_cost); |
Contributor
There was a problem hiding this comment.
Add explicit ORDER BY for VALUES fixtures used by an order-sensitive algorithm.
Line 112 (and the same pattern in Lines 132, 149, 171, 191, 253) feeds pgr_edgeColoring from unordered VALUES scans. Since this test file already proves output changes with edge order (q6/q7), missing ORDER BY can make expected color-count assertions flaky across planners/versions.
Proposed fix
PREPARE q8 AS
SELECT id, source, target, cost, reverse_cost
FROM (VALUES
(1, 3, 6, 20, 15),
(2, 3, 8, 10, -10),
(3, 6, 8, -1, 12))
-AS t(id, source, target, cost, reverse_cost);
+AS t(id, source, target, cost, reverse_cost)
+ORDER BY id ASC;Apply the same ORDER BY id ASC pattern to q9, q10, q11, q12, and q15.
Also applies to: 130-138, 148-152, 171-174, 191-199, 253-256
🤖 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 `@pgtap/coloring/edgeColoring/edge_cases.pg` around lines 110 - 116, The VALUES
fixtures feeding pgr_edgeColoring are unordered and can produce flaky results;
update each PREPARE statement (e.g., PREPARE q8, q9, q10, q11, q12, q15) that
selects FROM (VALUES ...) AS t(...) to wrap the VALUES subquery with an explicit
ORDER BY id ASC so the edge rows are deterministically ordered before calling
pgr_edgeColoring; ensure every VALUES-based query in this file using ids (the
same pattern around q8/q9/q10/q11/q12/q15) gets the ORDER BY id ASC applied.
Comment on lines
258
to
+259
| RETURN QUERY | ||
| SELECT is((SELECT count(DISTINCT color)::INTEGER FROM pgr_edgeColoring('q15')), 1, 'One color is required'); | ||
|
|
||
| SELECT is((SELECT count(DISTINCT color)::INTEGER FROM pgr_edgeColoring('q15')), 1, 'q15: One color is used'); |
Contributor
There was a problem hiding this comment.
Strengthen q15 to validate output cardinality, not only color uniqueness.
Line 259 only checks count(DISTINCT color) = 1; it does not prove both disconnected edges are returned. A partial/missing-row regression could slip through.
Proposed fix (single TAP assertion, no plan change)
RETURN QUERY
-SELECT is((SELECT count(DISTINCT color)::INTEGER FROM pgr_edgeColoring('q15')), 1, 'q15: One color is used');
+SELECT ok(
+ (SELECT count(*) = 2 AND count(DISTINCT color) = 1 FROM pgr_edgeColoring('q15')),
+ 'q15: Two rows are returned and one color is used'
+);📝 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.
Suggested change
| RETURN QUERY | |
| SELECT is((SELECT count(DISTINCT color)::INTEGER FROM pgr_edgeColoring('q15')), 1, 'One color is required'); | |
| SELECT is((SELECT count(DISTINCT color)::INTEGER FROM pgr_edgeColoring('q15')), 1, 'q15: One color is used'); | |
| RETURN QUERY | |
| SELECT ok( | |
| (SELECT count(*) = 2 AND count(DISTINCT color) = 1 FROM pgr_edgeColoring('q15')), | |
| 'q15: Two rows are returned and one color is used' | |
| ); |
🤖 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 `@pgtap/coloring/edgeColoring/edge_cases.pg` around lines 258 - 259, The q15
test only asserts count(DISTINCT color)=1 but not that both disconnected edges
are returned; update the single TAP assertion for q15 to validate both output
cardinality and color uniqueness in one check against pgr_edgeColoring('q15') —
e.g., replace the current distinct-color-only assertion with a single assertion
that ensures the result set has two rows (both edges returned) and that
count(DISTINCT color)=1, referencing pgr_edgeColoring('q15') and keeping it as
one TAP is() assertion.
Comment on lines
+46
to
+48
| std::vector<boost::default_color_type> partition(graph.num_vertices()); | ||
| auto partition_map = | ||
| make_iterator_property_map(partition.begin(), boost::get(boost::vertex_index, graph.graph)); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify remaining wrapper-style vertex count access in C++ sources
rg -nP --type=cpp --type=h '\b[a-zA-Z_][a-zA-Z0-9_]*\.num_vertices\s*\('Repository: pgRouting/pgrouting
Length of output: 5384
🏁 Script executed:
cat -n src/coloring/bipartite.cpp | head -60 | tail -35Repository: pgRouting/pgrouting
Length of output: 1462
🏁 Script executed:
# Get the function signature and graph type
sed -n '30,50p' src/coloring/bipartite.cppRepository: pgRouting/pgrouting
Length of output: 634
🏁 Script executed:
# Search for graph member structure or class definition
rg -B5 -A5 'struct.*graph|class.*graph' src/coloring/bipartite.cpp | head -40Repository: pgRouting/pgrouting
Length of output: 45
🏁 Script executed:
# Check the graph type passed to get_bipartition
rg -B10 'get_bipartition' src/coloring/bipartite.cpp | head -30Repository: pgRouting/pgrouting
Length of output: 573
🏁 Script executed:
find include -name "*.hpp" -o -name "*.h" | xargs grep -l "class UndirectedGraph" | head -5Repository: pgRouting/pgrouting
Length of output: 45
🏁 Script executed:
rg -A10 "class UndirectedGraph" include/Repository: pgRouting/pgrouting
Length of output: 45
🏁 Script executed:
rg -A10 "struct UndirectedGraph|typedef.*UndirectedGraph" include/Repository: pgRouting/pgrouting
Length of output: 45
🏁 Script executed:
# Check base_graph.hpp for class definition
rg -A15 "class.*Graph\s*{" include/cpp_common/base_graph.hpp | head -50Repository: pgRouting/pgrouting
Length of output: 159
🏁 Script executed:
# Search for num_vertices method in UndirectedGraph
rg "num_vertices\s*\(" include/ | grep -i "undirected\|base" | head -10Repository: pgRouting/pgrouting
Length of output: 432
🏁 Script executed:
# Get the full class definition in base_graph.hpp
sed -n '1,100p' include/cpp_common/base_graph.hpp | grep -A50 "class.*Graph"Repository: pgRouting/pgrouting
Length of output: 45
🏁 Script executed:
# Check what member holds the boost graph
rg -B5 -A5 'return boost::num_vertices\(graph\)' include/cpp_common/base_graph.hppRepository: pgRouting/pgrouting
Length of output: 336
🏁 Script executed:
# Search for the member declaration
rg '\bgraph\s*;' include/cpp_common/base_graph.hpp | head -5Repository: pgRouting/pgrouting
Length of output: 110
🏁 Script executed:
# Find UndirectedGraph definition - it might be a typedef or template
rg "UndirectedGraph" include/ | grep -E "typedef|using|class|struct" | head -20Repository: pgRouting/pgrouting
Length of output: 237
🏁 Script executed:
# Check the header file referenced in bipartite.cpp
cat include/coloring/bipartite.hppRepository: pgRouting/pgrouting
Length of output: 1504
🏁 Script executed:
# Find UndirectedGraph - check cpp_common for typedef
rg "UndirectedGraph\s*=" include/cpp_common/Repository: pgRouting/pgrouting
Length of output: 45
🏁 Script executed:
# Find where UndirectedGraph is defined as a typedef
rg "using UndirectedGraph|typedef.*UndirectedGraph" include/Repository: pgRouting/pgrouting
Length of output: 45
🏁 Script executed:
# Check the entire namespace for UndirectedGraph
rg "namespace pgrouting" include/cpp_common/ -A10 | grep -A5 "UndirectedGraph"Repository: pgRouting/pgrouting
Length of output: 45
🏁 Script executed:
# Search all files for UndirectedGraph definition
find . -type f \( -name "*.hpp" -o -name "*.h" \) -exec grep -l "UndirectedGraph" {} \; | head -10Repository: pgRouting/pgrouting
Length of output: 495
🏁 Script executed:
# Check base_graph.hpp for template or typedef after the Graph class
cat include/cpp_common/base_graph.hpp | tail -100Repository: pgRouting/pgrouting
Length of output: 3402
🏁 Script executed:
# Look for pgrouting namespace and UndirectedGraph in namespace
rg -A20 "namespace pgrouting" include/cpp_common/base_graph.hpp | tail -50Repository: pgRouting/pgrouting
Length of output: 670
🏁 Script executed:
# Check the Graph template class definition
rg -B5 "class Graph" include/cpp_common/base_graph.hpp | head -30Repository: pgRouting/pgrouting
Length of output: 45
🏁 Script executed:
# Find what UndirectedGraph is - likely template instantiation
rg "UndirectedGraph" include/cpp_common/ -B2 -A2Repository: pgRouting/pgrouting
Length of output: 1193
🏁 Script executed:
# Check the graph types header
cat include/cpp_common/graph_types.hpp 2>/dev/null || echo "File not found"Repository: pgRouting/pgrouting
Length of output: 78
🏁 Script executed:
# Look in the namespace for the typedef
sed -n '500,600p' include/cpp_common/base_graph.hppRepository: pgRouting/pgrouting
Length of output: 3764
Replace wrapper method with direct BGL call for vertex count.
Use boost::num_vertices(graph.graph) instead of graph.num_vertices() to align with the migration away from wrapper-base helpers and access the graph property directly through the Boost Graph Library.
♻️ Proposed refactor
- std::vector<boost::default_color_type> partition(graph.num_vertices());
+ std::vector<boost::default_color_type> partition(boost::num_vertices(graph.graph));📝 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.
Suggested change
| std::vector<boost::default_color_type> partition(graph.num_vertices()); | |
| auto partition_map = | |
| make_iterator_property_map(partition.begin(), boost::get(boost::vertex_index, graph.graph)); | |
| std::vector<boost::default_color_type> partition(boost::num_vertices(graph.graph)); | |
| auto partition_map = | |
| make_iterator_property_map(partition.begin(), boost::get(boost::vertex_index, graph.graph)); |
🤖 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/bipartite.cpp` around lines 46 - 48, Replace the wrapper call
graph.num_vertices() with the direct BGL function
boost::num_vertices(graph.graph) when sizing the partition vector and creating
partition_map; update the allocation of std::vector<boost::default_color_type>
partition(...) to use boost::num_vertices(graph.graph) and ensure
make_iterator_property_map still uses partition.begin() with
boost::get(boost::vertex_index, graph.graph) so the code references the
underlying Boost graph directly (symbols: partition, partition_map,
graph.num_vertices(), boost::num_vertices(graph.graph)).
Comment on lines
+52
to
+61
| void do_coloring( | ||
| const std::string &edges_sql, | ||
|
|
||
| Which which, | ||
|
|
||
| II_t_rt* &return_tuples, | ||
| size_t &return_count, | ||
| std::ostringstream &log, | ||
| std::ostringstream ¬ice, | ||
| std::ostringstream &err) { |
Contributor
There was a problem hiding this comment.
Initialize output parameters before any early return path.
do_coloring returns early in several branches before setting output params. Set safe defaults at entry so callers never observe stale return_tuples/return_count from prior state.
🛠️ Proposed fix
void do_coloring(
const std::string &edges_sql,
@@
std::ostringstream ¬ice,
std::ostringstream &err) {
std::string hint = "";
+ return_tuples = nullptr;
+ return_count = 0;Also applies to: 65-68, 80-84, 103-113
🧰 Tools
🪛 Cppcheck (2.20.0)
[style] 57-57: The function 'set_ids' is never used.
(unusedFunction)
[style] 59-59: The function 'dest_id' is never used.
(unusedFunction)
[style] 52-52: The function 'do_coloring' is never used.
(unusedFunction)
🤖 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 52 - 61, At the top of
do_coloring initialize the output parameters to safe defaults so callers never
see stale values: set return_tuples to null (no allocated pointer) and
return_count to 0 before any branching/early returns, and ensure any
early-return paths leave those defaults intact; update all branches that
currently return early (including the error/notice paths referenced around the
shown ranges) to rely on these initialized values rather than assuming they were
set later.
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 #3100 #3101
Changes proposed in this pull request:
@pgRouting/admins
Summary by CodeRabbit
Bug Fixes
Documentation
Tests