Max flow process and driver#3087
Conversation
WalkthroughThis PR extracts MaxFlow logic into a dedicated process and driver, adds Which enum values (MAXFLOW, PUSHRELABEL, BOYKOV, EDMONDSKARP), updates APIs/headers and utilities for Flow_t, and refactors callers (including max_flow.c) to use the new process/driver. Docs and locales updated. Changes
Sequence Diagram(s)sequenceDiagram
participant SRF as SRF entry (_pgr_maxflow)
participant Process as pgr_process_maxFlow
participant Driver as do_maxFlow
participant DB as PostgreSQL SPI
participant Utils as Utilities/IO
SRF->>Process: call pgr_process_maxFlow(edges_sql, combos_sql, starts, ends, Which)
Process->>DB: SPI_connect / execute queries (edges_sql/combos_sql)
Process->>Driver: invoke do_maxFlow(with SQL results, arrays, Which, &tuples, &count, logs)
Driver->>Utils: parse combinations -> source/sink sets
Driver->>Utils: load edges
Driver->>Driver: construct PgrFlowGraph(edges, sources, sinks, Which)
Driver->>Driver: run algorithm (switch on Which) -> compute Flow_t results
Driver->>Process: return Flow_t* tuples, count, logs/notices/errors
Process->>DB: SPI_finish / cleanup
Process->>SRF: return tuples/count to SRF
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@NEWS.md`:
- Line 24: The NEWS.md entry was manually edited but that file is generated;
remove the direct change from NEWS.md, add the bullet "*
[`#3086`](https://github.com/pgRouting/pgrouting/issues/3086): MaxFlow: create and
use a process and driver" into the notes source consumed by
tools/release-scripts/notes2news.pl (the release notes input file used by
notes2news.pl), then run tools/release-scripts/notes2news.pl to regenerate
NEWS.md so the entry is preserved in the generated output.
In `@src/max_flow/max_flow.c`:
- Around line 64-65: Before converting algorithm_id into enum Which via MAXFLOW
+ PG_GETARG_UINT32(3), validate the algorithm_id is within [0,3]; read the value
with PG_GETARG_UINT32(3) into a local uint32_t (e.g., algorithm_id), check if
(algorithm_id > 3) and if so raise a PostgreSQL error (ereport/elog) with a
clear message and abort; only after the check perform the cast to enum Which
(MAXFLOW + algorithm_id). Apply the same explicit bounds check for the second
occurrence that builds MAXFLOW + PG_GETARG_UINT32(3).
In `@src/max_flow/maxFlow_driver.cpp`:
- Line 124: The error message built in the max flow driver concatenates the text
and algorithm name with no separator; update the construction around err and
get_name(which) (the line using err << "Unknown flow function" <<
get_name(which)) to insert a clear separator such as ": " or " - " (and
optionally end with a newline) so the output reads e.g. "Unknown flow function:
<name>" instead of merging words.
In `@src/max_flow/maxFlow_process.cpp`:
- Line 2: Update the file header metadata string that currently reads "File:
max_flow.c" so it matches this source: change it to "File: maxFlow_process.cpp"
(or otherwise correct the filename string in the file header comment block) to
reflect the actual file name; locate the header comment near the top of
maxFlow_process.cpp and replace the incorrect "max_flow.c" token so the header
and filename are consistent.
In `@src/max_flow/maxflow.cpp`:
- Around line 50-61: The switch on `algorithm` currently falls through to an
empty `default` causing silent failures; update the `default` branch in the
switch (the one handling `algorithm` with cases `MAXFLOW`, `PUSHRELABEL`,
`BOYKOV`, `EDMONDSKARP`) to fail fast by raising/throwing an explicit error or
asserting (e.g. throw runtime_error or assert) that includes the invalid
`algorithm` value, ensuring any unsupported `Which` value is caught instead of
silently doing nothing; keep existing calls to
`insert_edges_push_relabel(edges)` and `insert_edges(edges)` unchanged.
ℹ️ Review info
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (21)
NEWS.mddoc/src/release_notes.rstinclude/c_common/enums.hinclude/cpp_common/combinations.hppinclude/cpp_common/to_postgres.hppinclude/cpp_common/utilities.hppinclude/drivers/maxFlow_driver.hppinclude/max_flow/maxflow.hppinclude/process/maxFlow_process.hlocale/en/LC_MESSAGES/pgrouting_doc_strings.polocale/pot/pgrouting_doc_strings.potsrc/cpp_common/combinations.cppsrc/cpp_common/to_postgres.cppsrc/cpp_common/utilities.cppsrc/max_flow/CMakeLists.txtsrc/max_flow/maxFlow_driver.cppsrc/max_flow/maxFlow_process.cppsrc/max_flow/max_flow.csrc/max_flow/max_flow_driver.cppsrc/max_flow/maxflow.cpptools/scripts/code_checker.sh
💤 Files with no reviewable changes (1)
- src/max_flow/max_flow_driver.cpp
| PG_GETARG_BOOL(4)? MAXFLOW : MAXFLOW + PG_GETARG_UINT32(3), | ||
| &result_tuples, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# 1. Find and examine the enum Which definition
rg -n -A 10 'enum\s+Which' include src --type c --type hRepository: pgRouting/pgrouting
Length of output: 3793
🏁 Script executed:
# 2. Examine the specific lines 64-65 and 79 in src/max_flow/max_flow.c
sed -n '50,85p' src/max_flow/max_flow.c | cat -nRepository: pgRouting/pgrouting
Length of output: 1414
🏁 Script executed:
# 3. Look for any validation of algorithm IDs before the problematic conversions
rg -n -B 5 -A 2 'PG_GETARG_UINT32.*[23]\)' src/max_flow/max_flow.c | head -50Repository: pgRouting/pgrouting
Length of output: 654
🏁 Script executed:
# 4. Check if pgr_process_maxFlow validates the Which enum parameter
fd -e c -e h | xargs rg -l 'pgr_process_maxFlow' | head -5Repository: pgRouting/pgrouting
Length of output: 120
🏁 Script executed:
# 5. Search for any validation of algorithm IDs in the wrapper or process functions
rg -n -B 3 -A 3 'PUSHRELABEL|EDMONDSKARP|BOYKOV' src/max_flow --type c --type hRepository: pgRouting/pgrouting
Length of output: 45
🏁 Script executed:
# Get the full enum Which definition from enums.h
cat -n include/c_common/enums.h | sed -n '28,100p'Repository: pgRouting/pgrouting
Length of output: 920
🏁 Script executed:
# Search for maxflow-related enum values or constants
rg -n 'MAXFLOW|maxflow' include/c_common/enums.h -A 5 -B 5Repository: pgRouting/pgrouting
Length of output: 324
🏁 Script executed:
# Check the pgr_process_maxFlow implementation for validation
cat include/process/maxFlow_process.hRepository: pgRouting/pgrouting
Length of output: 1769
🏁 Script executed:
# Search for any enum validation patterns in the maxflow codebase
rg -n 'enum Which' src/max_flow -A 5 -B 5 --type cRepository: pgRouting/pgrouting
Length of output: 45
🏁 Script executed:
# Find the pgr_process_maxFlow implementation to check for validation
fd -e c -type f | xargs rg -l 'pgr_process_maxFlow' | xargs grep -l 'pgr_process_maxFlow' | grep -v '.h'Repository: pgRouting/pgrouting
Length of output: 307
🏁 Script executed:
# Look at the maxFlow implementation file
find src/max_flow -name '*.c' -type f | head -3Repository: pgRouting/pgrouting
Length of output: 165
🏁 Script executed:
# Check if there's any validation of algorithm_id in the C code before enum conversion
rg -n 'algorithm_id|Which' src/max_flow/process/ -A 3 -B 3 --type c 2>/dev/null | head -100Repository: pgRouting/pgrouting
Length of output: 45
🏁 Script executed:
# Check the SQL function definitions for pgr_maxflow to see what algorithm IDs are documented
fd -name '*.sql' -type f | xargs rg -l 'maxflow' | head -3Repository: pgRouting/pgrouting
Length of output: 389
🏁 Script executed:
# Look at SQL schema to understand valid algorithm IDs
find . -path '*/sql/*' -name '*maxflow*' -o -path '*/sql/*' -name '*max_flow*' 2>/dev/null | head -5Repository: pgRouting/pgrouting
Length of output: 105
🏁 Script executed:
# Examine the maxFlow_process.cpp implementation
wc -l src/max_flow/maxFlow_process.cppRepository: pgRouting/pgrouting
Length of output: 98
🏁 Script executed:
# Look at the beginning of maxFlow_process.cpp to see validation logic
head -150 src/max_flow/maxFlow_process.cpp | tail -100Repository: pgRouting/pgrouting
Length of output: 2578
🏁 Script executed:
# Search for validation of the Which enum parameter in maxFlow_process
rg -n 'Which|algorithm|MAXFLOW' src/max_flow/maxFlow_process.cpp -A 2 -B 2 | head -80Repository: pgRouting/pgrouting
Length of output: 229
🏁 Script executed:
# Check the SQL definition to understand expected algorithm IDs
cat ./sql/max_flow/_maxflow.sql | head -200Repository: pgRouting/pgrouting
Length of output: 2188
🏁 Script executed:
# Check the do_maxFlow driver implementation for validation
fd -name 'maxFlow_driver.hpp' -o -name 'maxFlow_driver.cpp' | head -3Repository: pgRouting/pgrouting
Length of output: 293
🏁 Script executed:
# Look for any validation in the driver
find src/max_flow -name '*driver*' -type fRepository: pgRouting/pgrouting
Length of output: 230
🏁 Script executed:
# Check if there's any bounds checking on algorithm parameter in the wrapper
sed -n '1,90p' src/max_flow/max_flow.cRepository: pgRouting/pgrouting
Length of output: 2911
🏁 Script executed:
# Confirm the exact algorithm ID to enum mapping expected by SQL documentation
find . -path '*/doc/*' -o -path '*/sql/*' | xargs grep -l 'algorithm.*1\|PUSHRELABEL\|EDMONDSKARP' 2>/dev/null | head -3Repository: pgRouting/pgrouting
Length of output: 174
🏁 Script executed:
# Check the maxFlow_driver.cpp implementation
head -200 src/max_flow/maxFlow_driver.cppRepository: pgRouting/pgrouting
Length of output: 4345
🏁 Script executed:
# Search for validation logic in the driver implementation related to "which" parameter
rg -n 'which|PUSHRELABEL|EDMONDSKARP|BOYKOV' src/max_flow/maxFlow_driver.cpp -B 3 -A 3 | head -100Repository: pgRouting/pgrouting
Length of output: 1207
🏁 Script executed:
# Check the documentation to understand what algorithm IDs map to what enum values
cat ./doc/spanningTree/prim-family.rst | grep -A 10 'algorithm' | head -20Repository: pgRouting/pgrouting
Length of output: 1042
🏁 Script executed:
# Look for algorithm parameter documentation in maxflow documentation
find . -path '*/doc/*' -name '*maxflow*' -o -path '*/doc/*' -name '*max_flow*' | xargs cat 2>/dev/null | head -200Repository: pgRouting/pgrouting
Length of output: 45
🏁 Script executed:
# Verify the enum values more explicitly by checking their numeric positions
python3 << 'EOF'
# enum Which with DIJKSTRADD = 540 as the last explicit value before maxflow enums
# MAXFLOW, PUSHRELABEL, BOYKOV, EDMONDSKARP are consecutive auto-numbered enums
maxflow_base = 541
enums = {
'MAXFLOW': maxflow_base,
'PUSHRELABEL': maxflow_base + 1,
'BOYKOV': maxflow_base + 2,
'EDMONDSKARP': maxflow_base + 3,
}
print("Expected enum values based on enum.h:")
for name, value in enums.items():
print(f" {name} = {value}")
print("\nConversion logic in max_flow.c:")
print(" algorithm_id = 0 => MAXFLOW + 0 = {} (MAXFLOW)".format(maxflow_base))
print(" algorithm_id = 1 => MAXFLOW + 1 = {} (PUSHRELABEL)".format(maxflow_base + 1))
print(" algorithm_id = 2 => MAXFLOW + 2 = {} (BOYKOV)".format(maxflow_base + 2))
print(" algorithm_id = 3 => MAXFLOW + 3 = {} (EDMONDSKARP)".format(maxflow_base + 3))
print(" algorithm_id >= 4 => Invalid enum value (no corresponding case in switch)")
print("\nDriver validation (maxFlow_driver.cpp lines 112-126):")
print(" - Accepts: MAXFLOW, PUSHRELABEL, BOYKOV, EDMONDSKARP")
print(" - Invalid values hit 'default' case with error message")
print("\nConclusion: Enum arithmetic works if algorithm_id is 0-3, but:")
print(" - No explicit bounds check at wrapper level before enum conversion")
print(" - Relies on default case in driver to catch invalid values")
print(" - Coupling to enum ordinal layout is implicit and fragile")
EOFRepository: pgRouting/pgrouting
Length of output: 908
🏁 Script executed:
# Check if there's any documentation on valid algorithm_id values in SQL wrappers
find ./doc -name '*maxflow*' -o -name '*max_flow*' 2>/dev/null | xargs cat 2>/dev/null | grep -i 'algorithm' -A 5 -B 5 | head -100Repository: pgRouting/pgrouting
Length of output: 45
🏁 Script executed:
# Check for any recent validation changes or related issues
git log --oneline --all -- src/max_flow/max_flow.c 2>/dev/null | head -10Repository: pgRouting/pgrouting
Length of output: 494
Add explicit bounds checking on algorithm ID before enum conversion.
Lines 64 and 79 convert algorithm_id to enum Which via arithmetic (MAXFLOW + algorithm_id) without explicit validation. While the driver's switch statement catches invalid values, the lack of boundary validation at the wrapper level couples behavior to enum ordinal layout and leaves error handling implicit. Add an explicit bounds check before conversion.
The suggested fix should validate that the algorithm_id is in the valid range [0, 3]:
✅ Explicit bounds validation pattern
+static enum Which
+get_maxflow_which(uint32 algorithm_id, bool only_flow) {
+ if (only_flow) return MAXFLOW;
+
+ if (algorithm_id > 3) {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("Invalid maxflow algorithm id: %u", algorithm_id)));
+ }
+ return (enum Which)(MAXFLOW + algorithm_id);
+}
...
- PG_GETARG_BOOL(4)? MAXFLOW : MAXFLOW + PG_GETARG_UINT32(3),
+ get_maxflow_which(PG_GETARG_UINT32(3), PG_GETARG_BOOL(4)),
...
- PG_GETARG_BOOL(3)? MAXFLOW : MAXFLOW + PG_GETARG_UINT32(2),
+ get_maxflow_which(PG_GETARG_UINT32(2), PG_GETARG_BOOL(3)),Also applies to: line 79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/max_flow/max_flow.c` around lines 64 - 65, Before converting algorithm_id
into enum Which via MAXFLOW + PG_GETARG_UINT32(3), validate the algorithm_id is
within [0,3]; read the value with PG_GETARG_UINT32(3) into a local uint32_t
(e.g., algorithm_id), check if (algorithm_id > 3) and if so raise a PostgreSQL
error (ereport/elog) with a clear message and abort; only after the check
perform the cast to enum Which (MAXFLOW + algorithm_id). Apply the same explicit
bounds check for the second occurrence that builds MAXFLOW +
PG_GETARG_UINT32(3).
| max_flow = digraph.boykov_kolmogorov(); | ||
| break; | ||
| default: | ||
| err << "Unknown flow function" << get_name(which); |
There was a problem hiding this comment.
Improve unknown-algorithm error message formatting.
The message concatenates without a separator ("Unknown flow function" << get_name(which)), which hurts readability.
🧾 Proposed fix
- err << "Unknown flow function" << get_name(which);
+ err << "Unknown flow function: " << get_name(which);📝 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.
| err << "Unknown flow function" << get_name(which); | |
| err << "Unknown flow function: " << get_name(which); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/max_flow/maxFlow_driver.cpp` at line 124, The error message built in the
max flow driver concatenates the text and algorithm name with no separator;
update the construction around err and get_name(which) (the line using err <<
"Unknown flow function" << get_name(which)) to insert a clear separator such as
": " or " - " (and optionally end with a newline) so the output reads e.g.
"Unknown flow function: <name>" instead of merging words.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/max_flow/maxFlow_process.cpp`:
- Around line 79-83: The error path only clears *result_count when
*result_tuples is non-null; change the logic so that whenever err.str() is
non-empty we always set (*result_count) = 0 (and free/reset *result_tuples if
non-null). In other words, in the block checking if (!err.str().empty()) ensure
you assign (*result_count) = 0 unconditionally and only call pfree/clear
(*result_tuples) when (*result_tuples) is non-null to normalize the result_count
on every error path.
- Around line 60-87: Wrap the code between pgr_SPI_connect() and
pgr_SPI_finish() in a try-catch that guarantees cleanup: call do_maxFlow(...),
time_msg(...), and pgrouting::report_messages(...) inside the try; in the
catch(std::exception& e) and catch(...) blocks ensure you free (*result_tuples)
and set (*result_tuples)=nullptr and (*result_count)=0 if non-null, call
pgrouting::report_messages(log, notice, err) (and append the caught error
message into err for std::exception), then call pgr_SPI_finish() before
rethrowing or converting to a PG-compatible error; finally ensure
pgr_SPI_finish() is also called after the try path (e.g., in a finally-like
scope) so SPI cleanup always runs regardless of exceptions.
| pgr_SPI_connect(); | ||
|
|
||
| std::ostringstream log; | ||
| std::ostringstream err; | ||
| std::ostringstream notice; | ||
|
|
||
| clock_t start_t = clock(); | ||
| pgrouting::drivers::do_maxFlow( | ||
| edges_sql? edges_sql : "", | ||
| combinations_sql? combinations_sql : "", | ||
| starts, ends, | ||
|
|
||
| which, | ||
| (*result_tuples), (*result_count), | ||
| log, notice, err); | ||
|
|
||
| auto name = std::string(" processing ") + pgrouting::get_name(which); | ||
| time_msg(name.c_str(), start_t, clock()); | ||
|
|
||
| if (!err.str().empty() && (*result_tuples)) { | ||
| pfree(*result_tuples); | ||
| (*result_tuples) = nullptr; | ||
| (*result_count) = 0; | ||
| } | ||
|
|
||
| pgrouting::report_messages(log, notice, err); | ||
| pgr_SPI_finish(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "maxFlow_process.cpp" -type fRepository: pgRouting/pgrouting
Length of output: 97
🏁 Script executed:
cat -n ./src/max_flow/maxFlow_process.cppRepository: pgRouting/pgrouting
Length of output: 3196
🏁 Script executed:
find . -name "maxFlow_process.h" -type fRepository: pgRouting/pgrouting
Length of output: 98
🏁 Script executed:
cat -n ./include/process/maxFlow_process.hRepository: pgRouting/pgrouting
Length of output: 2217
🏁 Script executed:
find ./src -name "*_process.cpp" -type f | head -10Repository: pgRouting/pgrouting
Length of output: 317
🏁 Script executed:
# Check a few similar process files for exception handling patterns
for file in ./src/allpairs/allpairs_process.cpp ./src/astar/astar_process.cpp ./src/dijkstra/shortestPath_process.cpp; do
echo "=== $file ==="
grep -n "try\|catch" "$file" || echo "No try-catch found"
doneRepository: pgRouting/pgrouting
Length of output: 249
🏁 Script executed:
# Search across all process files for any try-catch patterns
grep -r "try\|catch" ./src --include="*_process.cpp"Repository: pgRouting/pgrouting
Length of output: 45
🏁 Script executed:
rg "pgr_SPI_finish" --max-count 20 -A 3 -B 1Repository: pgRouting/pgrouting
Length of output: 11110
🏁 Script executed:
sed -n '1,50p' ./src/common/postgres_connection.c | head -30Repository: pgRouting/pgrouting
Length of output: 1149
🏁 Script executed:
rg "pgr_SPI_finish\(" ./src/common/postgres_connection.c -A 5Repository: pgRouting/pgrouting
Length of output: 236
🏁 Script executed:
find . -name "maxFlow_driver.hpp" -type fRepository: pgRouting/pgrouting
Length of output: 99
🏁 Script executed:
cat -n ./include/drivers/maxFlow_driver.hpp | head -100Repository: pgRouting/pgrouting
Length of output: 2134
🏁 Script executed:
find . -name "*maxFlow_driver.cpp" -type fRepository: pgRouting/pgrouting
Length of output: 96
🏁 Script executed:
wc -l ./src/max_flow/maxFlow_driver.cppRepository: pgRouting/pgrouting
Length of output: 100
🏁 Script executed:
cat -n ./src/max_flow/maxFlow_driver.cppRepository: pgRouting/pgrouting
Length of output: 5374
🏁 Script executed:
# Check if there are other *_process files (especially C++ ones) with try-catch at the entry point level
find ./src -name "*_process.cpp" -exec grep -l "try\|catch" {} \;Repository: pgRouting/pgrouting
Length of output: 45
Make SPI lifecycle exception-safe in this C-facing process entrypoint.
The function is declared extern "C" and lacks exception safety. If an exception occurs during do_maxFlow(), time_msg(), or report_messages(), both pgrouting::report_messages() and pgr_SPI_finish() will be skipped. Since pgr_SPI_finish() performs critical SPI resource cleanup (calling SPI_finish() and reporting errors), an unhandled exception will leave the connection in an undefined state.
Although do_maxFlow() has internal exception handling, exceptions from other callsites in this function or unhandled exceptions bypassing internal catches can still escape to the C-facing boundary.
Wrap the processing logic in a try-catch block to guarantee cleanup:
Proposed fix
pgr_SPI_connect();
+ try {
std::ostringstream log;
std::ostringstream err;
std::ostringstream notice;
@@
auto name = std::string(" processing ") + pgrouting::get_name(which);
time_msg(name.c_str(), start_t, clock());
@@
- pgrouting::report_messages(log, notice, err);
- pgr_SPI_finish();
+ pgrouting::report_messages(log, notice, err);
+ } catch (...) {
+ /*
+ * Keep C-facing boundary safe and guarantee SPI cleanup/reporting path.
+ * Consider mapping to your standard internal error message helper.
+ */
+ std::ostringstream err;
+ err << "Unexpected exception in pgr_process_maxFlow";
+ std::ostringstream log;
+ std::ostringstream notice;
+ pgrouting::report_messages(log, notice, err);
+ }
+ pgr_SPI_finish();
}🧰 Tools
🪛 Cppcheck (2.19.0)
[style] 72-72: The function 'pgr_alloc' is never used.
(unusedFunction)
[style] 83-83: The function 'pgr_free' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/max_flow/maxFlow_process.cpp` around lines 60 - 87, Wrap the code between
pgr_SPI_connect() and pgr_SPI_finish() in a try-catch that guarantees cleanup:
call do_maxFlow(...), time_msg(...), and pgrouting::report_messages(...) inside
the try; in the catch(std::exception& e) and catch(...) blocks ensure you free
(*result_tuples) and set (*result_tuples)=nullptr and (*result_count)=0 if
non-null, call pgrouting::report_messages(log, notice, err) (and append the
caught error message into err for std::exception), then call pgr_SPI_finish()
before rethrowing or converting to a PG-compatible error; finally ensure
pgr_SPI_finish() is also called after the try path (e.g., in a finally-like
scope) so SPI cleanup always runs regardless of exceptions.
| if (!err.str().empty() && (*result_tuples)) { | ||
| pfree(*result_tuples); | ||
| (*result_tuples) = nullptr; | ||
| (*result_count) = 0; | ||
| } |
There was a problem hiding this comment.
Normalize *result_count on every error path.
When err is set and *result_tuples is null, *result_count is currently left untouched.
🔧 Proposed fix
- if (!err.str().empty() && (*result_tuples)) {
- pfree(*result_tuples);
- (*result_tuples) = nullptr;
- (*result_count) = 0;
+ if (!err.str().empty()) {
+ if (*result_tuples) {
+ pfree(*result_tuples);
+ (*result_tuples) = nullptr;
+ }
+ (*result_count) = 0;
}📝 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.
| if (!err.str().empty() && (*result_tuples)) { | |
| pfree(*result_tuples); | |
| (*result_tuples) = nullptr; | |
| (*result_count) = 0; | |
| } | |
| if (!err.str().empty()) { | |
| if (*result_tuples) { | |
| pfree(*result_tuples); | |
| (*result_tuples) = nullptr; | |
| } | |
| (*result_count) = 0; | |
| } |
🧰 Tools
🪛 Cppcheck (2.19.0)
[style] 83-83: The function 'pgr_free' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/max_flow/maxFlow_process.cpp` around lines 79 - 83, The error path only
clears *result_count when *result_tuples is non-null; change the logic so that
whenever err.str() is non-empty we always set (*result_count) = 0 (and
free/reset *result_tuples if non-null). In other words, in the block checking if
(!err.str().empty()) ensure you assign (*result_count) = 0 unconditionally and
only call pfree/clear (*result_tuples) when (*result_tuples) is non-null to
normalize the result_count on every error path.
2 participants
Fixes #3086 .
Changes proposed in this pull request:
@pgRouting/admins
Summary by CodeRabbit
New Features
Documentation