Creating v0.1.0 #2
Conversation
WalkthroughAdds initial project scaffolding for pgvroom v0.1.0: build system (CMake, find modules), CI workflows, documentation (Sphinx, Doxygen, reST pages), repository configs, helper scripts, and a broad set of public headers defining C/C++ APIs and data types for VROOM integration. Includes NEWS and docqueries fixtures. Changes
Sequence Diagram(s)(no diagram) Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 77
🔭 Outside diff range comments (1)
.github/workflows/locale.yml (1)
88-106: Prevent infinite self-trigger loops when pushing locale updatesThis workflow pushes to doc/ and locale/, which re-triggers itself. Add a guard to skip when the actor is the bot or when commit message is the auto-update.
jobs: update-locale: name: Update Locale runs-on: ubuntu-latest + if: github.actor != 'github-actions[bot]' @@ - name: Update locale run: | bash .github/scripts/update_locale.sh # Add the files, commit and push - git diff --staged --quiet || git commit -m "Update locale: commit for hash ${{ env.GIT_HASH }}" + git add -A + git diff --staged --quiet || git commit -m "chore(locale): update for ${{ env.GIT_HASH }}" git restore . # Remove the unstaged changes before rebasing git pushOptionally also add a commit-message filter to on.push to ignore these commits.
🧹 Nitpick comments (74)
include/cpp_common/assert.hpp (4)
95-101: Make concatenation explicit to avoid const char + std::string surprises*Relying on operator+(const char*, const std::string&) is legal but non-obvious. Wrapping the literal in std::string improves clarity and avoids overload pitfalls.
- : throw AssertFailedException( \ - "AssertFailedException: " __STRING(expr) \ - " at " __FILE__ ":" __TOSTRING(__LINE__) + get_backtrace() ) ) + : throw AssertFailedException( \ + std::string("AssertFailedException: " __STRING(expr) \ + " at " __FILE__ ":" __TOSTRING(__LINE__)) + get_backtrace() ) )
118-124: Same concatenation refinement for pgassertwmMirror the explicit std::string construction here as well.
- : throw AssertFailedException( \ - "AssertFailedException: " __STRING(expr) \ - " at " __FILE__ ":" __TOSTRING(__LINE__) + get_backtrace(msg) ) ) + : throw AssertFailedException( \ + std::string("AssertFailedException: " __STRING(expr) \ + " at " __FILE__ ":" __TOSTRING(__LINE__)) + get_backtrace(msg) ) )
70-72: Fix include in example (header name)The header is assert.hpp, not assert.h. Use the public include path shown in this repo.
- #include "assert.h" + #include <cpp_common/assert.hpp>
108-112: Correct macro name in example and missing semicolonThe message-bearing macro is pgassertwm. Also add the missing semicolon after the stream insertion.
- pgassert(2+2 == 5, "Expected a 4 as result"); + pgassertwm(2+2 == 5, "Expected a 4 as result"); std::ostringstream log; - log << "Expected a 4 as result" - pgassert(2+2 == 5, log.str()); + log << "Expected a 4 as result"; + pgassertwm(2+2 == 5, log.str());include/c_common/time_msg.h (1)
31-34: Add a brief API doc (Doxygen) to clarify usageDocument expected inputs (start_t/end_t from clock()), log level expectations (DEBUG2), and that msg is not modified.
include/c_common/e_report.h (1)
31-36: Document pointer ownership and side-effectsvro_global_report may free *log_msg and *notice_msg when no error (per src/common/e_report.c). Please document:
- Inputs: pointers may be NULL
- Ownership: frees log/notice when error_msg is NULL; does not free error_msg
- Thread-safety: not thread-safe (uses PG memory context)
I can add a Doxygen block with precise ownership semantics if you’d like.
include/c_common/debug_macro.h (1)
30-37: Provide a VRO-prefixed alias for consistency with the project namingNon-breaking alias; helps migration toward vro_ naming while keeping PGR_DBG.
#endif +#ifndef VRO_DBG +#define VRO_DBG(...) PGR_DBG(__VA_ARGS__) +#endif + #endif // INCLUDE_C_COMMON_DEBUG_MACRO_H_include/cpp_common/messages.hpp (3)
72-76: Doxygen mismatch for has_error()The brief says “gets the contents of notice message” but the return docs refer to error emptiness. Update the brief to reflect the predicate nature.
90-96: Consider encapsulation: make streams private and expose stream accessorsPublic mutable members invite misuse. Prefer private members with accessors like std::ostream& log_stream().
I can refactor the API to add log_stream(), notice_stream(), error_stream() while keeping getters and clear().
100-106: Avoid generic macro names; add prefixed aliases to prevent collisionsENTERING/EXITING are very broad and can clash. Add VRO-prefixed forms; keep existing for compatibility.
#if defined(__MINGW32__) || defined(_MSC_VER) #define ENTERING(x) #define EXITING(x) +#define VRO_ENTERING(x) +#define VRO_EXITING(x) #else #define ENTERING(x) x << "\n--> " << __PRETTY_FUNCTION__ << "\n" #define EXITING(x) x << "\n<-- " << __PRETTY_FUNCTION__ << "\n" +#define VRO_ENTERING(x) ENTERING(x) +#define VRO_EXITING(x) EXITING(x) #endifinclude/cpp_common/alloc.hpp (2)
34-45: Make C prototypes self-contained and consistentEnsure size_t is visible and simplify declarations; the stray standalone 'extern' is odd. Suggest tightening formatting for readability.
-extern "C" { - -extern -void* SPI_palloc(size_t size); - -extern void * -SPI_repalloc(void *pointer, size_t size); - -extern void -SPI_pfree(void *pointer); - -} +extern "C" { +void* SPI_palloc(size_t size); +void* SPI_repalloc(void* pointer, size_t size); +void SPI_pfree(void* pointer); +}
54-58: Docs contradict API usageComment says “free must occur in the C code”, yet a C++ free wrapper is provided below. Clarify whether freeing is expected from C++ via SPI_pfree or delegated to C.
include/cpp_common/identifier.hpp (1)
71-80: Minor doc fix: “Inherited functions” → “Member functions”These are member functions, not inherited ones.
include/c_types/typedefs.h (2)
31-33: Remove unused include to reduce header weightis not needed here.
-#include <cstring>
55-57: Remove unused include on the C side as well<string.h> is not used.
-#include <string.h>include/c_types/vroom_rt.h (1)
44-55: Docs: fix step_type description in the tableThe table says “Step sequence of the vehicle” for step_type; field comment below says “Type of the step”. Align the table row.
-step_type | Step sequence of the vehicle +step_type | Type of the stepinclude/cpp_common/identifiers.hpp (1)
81-87: Precondition checks for front()/back()/pop_front()These assume non-empty set. Add asserts or guards to prevent UB on empty sets, or document the precondition.
include/cpp_common/matrix_cell_t.hpp (2)
43-48: Use struct and default-initialize fields to avoid indeterminate values on default constructionThis is a simple POD; using struct and adding in-class initializers prevents accidental UB if default-constructed.
-class Matrix_cell_t { - public: - Id from_vid; /** @b departure node's identifier */ - Id to_vid; /** @b arrival node's identifier */ - TInterval cost; /** Travel Interval from departure to arrival */ -}; +struct Matrix_cell_t { + Id from_vid = 0; /** @b departure node's identifier */ + Id to_vid = 0; /** @b arrival node's identifier */ + TInterval cost = 0;/** Travel Interval from departure to arrival */ +};
41-47: Clarify units: “cost” vs “interval”Field name says cost but type/comment implies a time interval. Please align terminology (e.g., rename to duration or update docs).
include/cpp_common/undefPostgresDefine.hpp (1)
42-55: Doc nits: clarify Boost version and wordingMinor wording tweak to avoid ambiguity.
-It's affecting boost 83+ +This affects Boost 1.83+. @@ -#include <system files> +#include <standard headers>include/cpp_common/get_data.hpp (1)
56-56: Consider making the fetch batch size configurable and reduce the defaultA limit of 1,000,000 can spike memory/CPU. Suggest constexpr size with a smaller default or an overload/parameter to tune per use.
include/cpp_common/info.hpp (1)
35-65: Optional: use a scoped enum to avoid namespace pollution and enforce type-safetyIf feasible, migrate to enum class expectType : uint8_t and qualify uses. This is a larger change; consider for a future refactor.
include/cpp_common/base_matrix.hpp (2)
26-28: Consider using either header guards or#pragma once, not bothWhile not harmful, using both traditional header guards and
#pragma onceis redundant. Most modern compilers support#pragma once, so you could simplify to just one approach for consistency across the codebase.
110-111: Consider storingIdentifiers<Id>instead ofstd::vector<Id>for consistencyThe constructor accepts
Identifiers<Id>but internally storesstd::vector<Id>. This creates unnecessary duplication and potential inconsistency. SinceIdentifiers<Id>already provides the necessary interface (size(), empty(), iteration), consider storing it directly or documenting why the conversion is necessary.include/cpp_common/pgdata_fetchers.hpp (2)
30-32: Consider using either header guards or#pragma once, not bothSame as in base_matrix.hpp - using both is redundant. Consider standardizing across the codebase.
57-62: Add parameter names to function declarations for better readabilityThe function declarations lack parameter names, which reduces readability and makes it harder to understand the purpose of each parameter without looking at the implementation.
-Vroom_matrix_t fetch_matrix(const HeapTuple, const TupleDesc&, const std::vector<Info>&, bool); -Vroom_time_window_t fetch_timewindows(const HeapTuple, const TupleDesc&, const std::vector<Info>&, bool); -Vroom_job_t fetch_jobs(const HeapTuple, const TupleDesc&, const std::vector<Info>&, bool); -Vroom_break_t fetch_breaks(const HeapTuple, const TupleDesc&, const std::vector<Info>&, bool); -Vroom_shipment_t fetch_shipments(const HeapTuple, const TupleDesc&, const std::vector<Info>&, bool); -Vroom_vehicle_t fetch_vehicles(const HeapTuple, const TupleDesc&, const std::vector<Info>&, bool); +Vroom_matrix_t fetch_matrix(const HeapTuple tuple, const TupleDesc& tupdesc, const std::vector<Info>& info, bool strict); +Vroom_time_window_t fetch_timewindows(const HeapTuple tuple, const TupleDesc& tupdesc, const std::vector<Info>& info, bool strict); +Vroom_job_t fetch_jobs(const HeapTuple tuple, const TupleDesc& tupdesc, const std::vector<Info>& info, bool strict); +Vroom_break_t fetch_breaks(const HeapTuple tuple, const TupleDesc& tupdesc, const std::vector<Info>& info, bool strict); +Vroom_shipment_t fetch_shipments(const HeapTuple tuple, const TupleDesc& tupdesc, const std::vector<Info>& info, bool strict); +Vroom_vehicle_t fetch_vehicles(const HeapTuple tuple, const TupleDesc& tupdesc, const std::vector<Info>& info, bool strict);include/cpp_common/check_get_data.hpp (1)
30-32: Consider using either header guards or#pragma once, not bothConsistent with other header files - using both is redundant.
doc/forward.html (1)
1-15: Consider modernizing the HTML structure.The redirect implementation is functionally correct with good fallback mechanisms, but uses outdated XHTML 1.0 Transitional. Consider updating to HTML5 for better standards compliance and simplicity.
-<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" - "https://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> -<html xmlns="https://www.w3.org/1999/xhtml"> +<!DOCTYPE html> +<html lang="en">docqueries/dataCheck/data_check.pg (1)
3-10: Remove redundant schema qualification.Since the search_path is set to include 'example2', the explicit schema qualification in the SELECT statements is redundant and can be removed for cleaner code.
-SELECT count(*) FROM example2.shipments; -SELECT count(*) FROM example2.physical_vehicles; -SELECT count(*) FROM example2.vehicles; -SELECT count(*) FROM example2.depot; -SELECT count(*) FROM example2.locations; -SELECT count(*) FROM example2.timeMatrix; -SELECT count(*) FROM example2.timeMatrix WHERE assigned; -SELECT count(*) FROM example2.timeMatrix WHERE NOT assigned; +SELECT count(*) FROM shipments; +SELECT count(*) FROM physical_vehicles; +SELECT count(*) FROM vehicles; +SELECT count(*) FROM depot; +SELECT count(*) FROM locations; +SELECT count(*) FROM timeMatrix; +SELECT count(*) FROM timeMatrix WHERE assigned; +SELECT count(*) FROM timeMatrix WHERE NOT assigned;CONTRIBUTING.md (1)
3-3: Consider the style suggestion (optional).The static analysis tool suggests replacing "really glad" with a less overused intensifier, though this is a minor stylistic preference and doesn't affect the document's functionality.
CODE_OF_CONDUCT.md (1)
1-1: Replace bare HTTP URL with HTTPS Markdown link (markdownlint MD034).Keep the transcription intact, but format the intro link and prefer HTTPS.
-Contributors to pgRouting are expected to act respectfully toward others in accordance with the http://www.osgeo.org/code_of_conduct. +Contributors to pgRouting are expected to act respectfully toward others in accordance with the [OSGeo Code of Conduct](https://www.osgeo.org/code_of_conduct/).cmake/pgr/BuildType.cmake (1)
11-16: Expose allowed build types in CMake cache for better UX.Add STRINGS to the cache entry so GUIs and ccmake offer valid choices.
if(NOT CMAKE_BUILD_TYPE AND NOT CMAKE_CONFIGURATION_TYPES) message(STATUS "Setting build type to '${PROJECT_DEFAULT_BUILD_TYPE}' as none was specified.") set(CMAKE_BUILD_TYPE "${PROJECT_DEFAULT_BUILD_TYPE}" CACHE STRING "Choose the type of build." FORCE) + set_property(CACHE CMAKE_BUILD_TYPE PROPERTY STRINGS + Debug Release RelWithDebInfo MinSizeRel) endif().gitignore (1)
1-15: Consider a few additional ubiquitous ignore patterns.Keeps the list focused while covering common CMake and IDE artifacts.
build +build-*/ +cmake-build-*/ fix_typos code_linter src/version/_version.h vroom-v[0-9]* # files are ignored on gh-pages doxy .DS_Store .vagrant .directory *.swp +.idea/ +*.iml +compile_commands.json +CMakeCache.txt +CMakeFiles/ +Testing/.editorconfig (1)
52-56: Correct comment to match the section (HTML, not RST).Purely a readability nit.
-# RST files +# HTML files [*.html] indent_size = 2 indent_style = space.github/workflows/clang.yml (1)
84-93: Install clang-tidy to match USE_CLANG_TIDY=ON.CMake enables clang-tidy; ensure the tool is present.
sudo apt-get install -y \ - clang \ + clang clang-tidy \ libtap-parser-sourcehandler-pgtap-perl \ postgresql-${PGVER} \ postgresql-${PGVER}-pgtap \ postgresql-server-dev-${PGVER}.github/workflows/macos.yml (1)
102-107: Harden test step: ensure DB exists and role creation is idempotent.export PATH=$(brew --prefix)/Cellar/perl/$(perl -e 'print substr($^V, 1)')/bin:$PATH pg_ctl -D $(brew --prefix)/var/postgresql@14 start - createuser -s postgres - bash ./tools/testers/pg_prove_tests.sh -U postgres -p 5432 + createuser -s postgres || true + createdb ___pgvroom___pgtap___ || true + bash ./tools/testers/pg_prove_tests.sh -U postgres -p 5432 -d ___pgvroom___pgtap___.github/workflows/ubuntu.yml (1)
25-25: Remove empty tags section.The empty
tags: []section is unnecessary and flagged by actionlint. Either specify tag patterns or remove this section entirely.Apply this diff:
- tags: [].github/workflows/documentation.yml (1)
26-26: Remove empty tags section.Similar to the Ubuntu workflow, the empty
tags: []section should be removed as it's flagged by actionlint.Apply this diff:
- tags: []doc/_static/custom.css (1)
8-8: Consider accessibility of blue background.The
background-color: bluefor the content wrapper may create accessibility issues with text contrast. Consider using a more neutral or lighter shade..github/workflows/queries.yml (1)
30-30: Remove empty tags section.Another instance of the empty
tags: []section that should be removed.Apply this diff:
- tags: []NEWS.md (1)
8-8: Use proper markdown heading instead of bold emphasis.Line 8 uses emphasis (
**Extraction tasks**) instead of a proper markdown heading. Consider using a heading format for better document structure.-**Extraction tasks** +## Extraction tasksdoc/_templates/layout.html (1)
11-11: Consider alternatives to document.write() for modern compatibility.The inline
document.write()call is generally considered outdated and can cause issues in modern browsers, especially with async loading. Consider using DOM manipulation or template rendering instead.- <script> document.write(get_history('{{ pagename }}','{{ language }}')); </script> + <div id="page-history"></div> + <script> + document.addEventListener('DOMContentLoaded', function() { + document.getElementById('page-history').innerHTML = get_history('{{ pagename }}','{{ language }}'); + }); + </script>doc/general/functions.rst (1)
40-48: Minor indentation inconsistency in VROOM toctree.The VROOM toctree has inconsistent indentation (2 spaces vs 3 spaces used in Utilities section).
.. toctree:: - :hidden: + :hidden: - vro_vroom - vro_vroomJobs - vro_vroomShipments - vro_vroomPlain - vro_vroomJobsPlain - vro_vroomShipmentsPlain + vro_vroom + vro_vroomJobs + vro_vroomShipments + vro_vroomPlain + vro_vroomJobsPlain + vro_vroomShipmentsPlaindoc/_static/CMakeLists.txt (1)
8-11: Use COPYONLY for static/binary assetsAvoid substitution on CSS/JS/ICO (esp. binary .ico). COPYONLY is safer.
-foreach (f ${LOCAL_FILES}) - configure_file(${f} ${f} @ONLY) +foreach (f ${LOCAL_FILES}) + configure_file(${f} ${f} COPYONLY) list(APPEND LOCAL_DOC_FILES ${CMAKE_CURRENT_BINARY_DIR}/${f}) endforeach()doc/version/CMakeLists.txt (1)
7-10: Optional: use COPYONLY to prevent unintended @var@ substitutionIf these .rst files aren't templates, use COPYONLY. If you plan to inject vars (e.g., version), keep current behavior.
-foreach (f ${LOCAL_FILES}) - configure_file(${f} "${PGR_DOCUMENTATION_SOURCE_DIR}/${f}") +foreach (f ${LOCAL_FILES}) + configure_file(${f} "${PGR_DOCUMENTATION_SOURCE_DIR}/${f}" COPYONLY) list(APPEND LOCAL_DOC_FILES ${PGR_DOCUMENTATION_SOURCE_DIR}/${f}) endforeach()doc/_static/images/CMakeLists.txt (1)
2-5: Avoid duplicate favicon across _static and _static/imagesfavicon.ico is listed here and in doc/_static/CMakeLists.txt. Keep a single canonical copy to prevent confusion.
Would you prefer it to live in doc/_static/ or doc/_static/images/? I can propose a follow-up diff once you decide.
docqueries/vroom/vroomJobs.result (1)
1-11: Stabilize session settings for deterministic resultsConsider adding DateStyle (and potentially extra_float_digits if floats appear in other fixtures) to avoid locale/date formatting drift across CI images.
Example:
BEGIN; +SET DateStyle TO ISO, MDY; SET client_min_messages TO NOTICE; SET -SET TIMEZONE TO 'UTC'; +SET TIMEZONE TO 'UTC';.github/workflows/locale.yml (2)
27-31: Unused env variablesenv.os and env.vroom are defined but unused. Remove unless you plan to consume them.
env: release: Release - os: ubuntu-latest - vroom: 1.12.0
88-93: Set and use branch only if neededYou compute branch but never use it. Either remove or use it to restrict pushes to same branch.
- - name: Extract branch name and commit hash + - name: Extract commit hash run: | - branch=${GITHUB_REF#refs/heads/} git_hash=$(git rev-parse --short "$GITHUB_SHA") echo "GIT_HASH=$git_hash" >> $GITHUB_ENVdoc/version/vro_full_version.rst (1)
55-66: Fix column names/wording: PostgreSQL casing, and “Operating system”Minor doc nits for correctness and consistency.
``library`` ``TEXT`` Library name and version -``system`` ``TEXT`` Operative system -``postgreSQL`` ``TEXT`` pgsql used +``system`` ``TEXT`` Operating system +``PostgreSQL`` ``TEXT`` PostgreSQL usedconfiguration.conf (2)
9-14: Comment vs. ordering mismatchComment says “common must be first” but pg_controls precedes it. Clarify that pg_controls must precede all, then common, or move common to top.
-# common must be first can not be removed +# pg_controls must be first. common must follow and cannot be removed. pg_controls | N | Y | N common | Y | N | N
21-23: Add trailing newline and keep Y/N consistencyMinor hygiene to avoid tooling diffs.
utilities | N | Y | N +doc/general/sampledata.rst (2)
13-16: Clarify whether schema/table creation is includedThe text says “create a table,” but the includes look like data snippets. Consider rewording or add CREATE TABLE snippets/links.
-To be able to execute the sample queries, run the following SQL commands to create a table with a small network data set. +To execute the sample queries, load the sample data below into your database (tables and schema definitions are provided in the referenced vroomdata.sql).
69-75: Optional: add a note on using \copy for quick loadingA short tip helps users load fixtures faster.
.. literalinclude:: ../../tools/testers/vroomdata.sql :start-after: -- vroom MATRIX TABLE start :end-before: -- vroom MATRIX TABLE end + +.. note:: + You can use psql’s ``\copy`` to load these data blocks directly into your local database.Also applies to: 128-134
.github/scripts/test_signatures.sh (1)
1-11: Harden the script (strict mode, comments, portability)Add set -euo pipefail and tidy comments; also you don’t need to be at repo root thanks to rev-parse.
-#!/bin/bash - -# This test checks that signatures within mayors -# Call from the root of the repository +#!/bin/bash +set -euo pipefail + +# Purpose: ensure signatures are not removed within the same major (>0) +# Can be called from any directory @@ -popd > /dev/null || exit +popd > /dev/null || exit @@ -#mylicensecheck doc error=0 if [[ $missing ]]; then echo " ****************************************************" echo " *** Found removed signatures" echo " ****************************************************" echo "$missing" error=1 fiAlso applies to: 32-46
doc/vroom/CMakeLists.txt (2)
2-9: Initialize LOCAL_DOC_FILES to avoid accidental accumulationCMake variables are directory-scoped but can leak; initialize to keep this file self-contained.
-SET(LOCAL_FILES +set(LOCAL_FILES vro_vroom.rst @@ - ) + ) +set(LOCAL_DOC_FILES)
16-17: Use lowercase set for consistencyStyle nit to match the rest of the repository CMake style.
-set(PROJECT_DOC_FILES ${PROJECT_DOC_FILES} ${LOCAL_DOC_FILES} PARENT_SCOPE) -set(PROJECT_IMG_FILES ${PROJECT_IMG_FILES} PARENT_SCOPE) +set(PROJECT_DOC_FILES ${PROJECT_DOC_FILES} ${LOCAL_DOC_FILES} PARENT_SCOPE) +set(PROJECT_IMG_FILES ${PROJECT_IMG_FILES} PARENT_SCOPE)doc/conf.py.in (2)
44-49: PDF config present but rst2pdf extension not enabledYou configure pdf_documents/stylesheets, but rst2pdf builder isn’t enabled. Add the extension to avoid silent ignores during PDF builds.
extensions = [ 'sphinx.ext.todo', 'sphinx.ext.mathjax', 'sphinx.ext.graphviz', 'sphinx.ext.autosectionlabel', + 'rst2pdf.pdfbuilder', ]Also applies to: 157-165
89-103: Bootstrap theme options target v2 (legacy).sphinx_bootstrap_theme supports Bootstrap 3; using 'bootstrap_version': "2" is legacy and may render poorly. Consider moving to 3 (and revisit navbar_* options accordingly).
.github/workflows/files.yml (1)
7-7: Typos in workflow commentsFix minor typos for clarity.
-# - the release_notes and NEWS are consitent +# - the release_notes and NEWS are consistent @@ -# Does not makes a compilation +# Does not make a compilationAlso applies to: 11-11
docqueries/vroom/vroomJobs.pg (1)
1-3: Optionally bracket with BEGIN/ROLLBACK for reproducible capturesOther fixtures include transaction wrappers; consider adding BEGIN/ROLLBACK here for consistent outputs across runs.
Also applies to: 4-13, 15-39
doc/general/introduction.rst (4)
26-27: Grammar: duplicated “organization” and wording-pgvroom is a sub product of `pgRouting organization <https://pgrouting.org>`__ -organization. +pgvroom is a sub-project of the `pgRouting organization <https://pgrouting.org>`__.
47-49: License line: plural verb and anonymous linkUse “are” for plural and anonymous external link to avoid duplicate implicit targets.
- - The pgvroom Manual and examples is licensed under a `Creative Commons - Attribution-Share Alike 3.0 License - <https://creativecommons.org/licenses/by-sa/3.0/>`_. + - The pgvroom Manual and examples are licensed under a `Creative Commons + Attribution-Share Alike 3.0 License + <https://creativecommons.org/licenses/by-sa/3.0/>`__.
73-77: Name consistency and source of portPast/present sections list “Vicky Vergara”; here it says “Virginia Vergara”. Also, per PR objectives the port source is vrprouting.
-- Virginia Vergara +- Vicky Vergara @@ - - Porting Basic Code from pgvroom + - Porting basic code from vrprouting
86-88: Sponsor naming consistencyUse the same name/link style for “erosion” across sections.
-- `Erosion developers <https://www.erosion.dev/>`__ +- `erosion <https://www.erosion.dev/>`__ @@ -- Erosion developers +- erosionAlso applies to: 117-119
docqueries/vroom/vroomShipmentsPlain.pg (1)
36-37: Consider adding a TODO comment for the empty q3The empty q3 placeholder should have a TODO comment explaining what test case is planned here for better maintainability.
-/* -- q3 */ +/* -- q3 */ +/* TODO: Add test case for [describe intended test scenario] */.github/scripts/notes2news.pl (3)
26-28: Enhance usage message with more detailsThe usage message could be more descriptive to help users understand the script's purpose and requirements.
sub Usage { - die "Usage: notes2news.pl (from the root of the repository or pre-commit hook)\n"; + die "Usage: notes2news.pl\n" . + " Converts doc/general/release_notes.rst to NEWS.md\n" . + " Must be run from the repository root or as a pre-commit hook\n"; }
59-70: Consider more flexible header pattern matchingThe header conversion is hardcoded for specific patterns and might miss variations like "pgvroom 0.10.0" (double-digit versions).
- if ($line =~ m/^pgvroom [0-9]$/i) { + if ($line =~ m/^pgvroom \d+$/i) { print $ofh "# $line"; next; }; - if ($line =~ m/^pgvroom [0-9].[0-9]$/i) { + if ($line =~ m/^pgvroom \d+\.\d+$/i) { print $ofh "## $line"; next; }; - if ($line =~ m/^pgvroom [0-9].[0-9].[0-9] Release Notes$/i) { + if ($line =~ m/^pgvroom \d+\.\d+\.\d+ Release Notes$/i) { print $ofh "### $line"; next; };
168-171: Improve key/value extraction clarityThe current string manipulation using substr and index is hard to follow. Consider using regex for clearer extraction.
- my ($key) = substr($line, 4, index(substr($line, 4), "|")); - my ($value) = substr($line, index($line,"`")); - $value =~ s/\R//g; + # Extract key and value using regex: ".. |key| replace:: `value`" + if ($line =~ /^\.\.\s+\|([^|]+)\|\s+replace::\s+(.+)$/) { + my ($key, $value) = ($1, $2); + $value =~ s/\R//g; + $data{$key} = $value; + print "$key $data{$key} \n" if $DEBUG; + } - $data{$key} = $value; - print "$key $data{$key} \n" if $DEBUG;doc/general/installation.rst (1)
107-108: Address TODO comments in documentationThe documentation contains TODO items that need to be completed before release.
Would you like me to help determine the appropriate version numbers based on the CMakeLists.txt configuration?
Also applies to: 123-124
doc/vroom/vro_vroom.rst (2)
17-23: Fill in (or remove) the empty “Availability” rubric“Availability” is declared but has no content. Either remove it or add specifics (extension name, min version, required VROOM, function prefix).
Example content:
- Extension: pgvroom >= 0.1.0
- Requires VROOM 1.12.0
- Function prefix: vro_
124-126: Minor grammar: “return back the data”Tighten wording.
- - Gives the best possible solution within a time limit. Note that some - additional seconds may be required to return back the data. + - Gives the best possible solution within a time limit. Note that some + additional seconds may be required to return the data.doc/CMakeLists.txt (2)
154-160: Quoting for -D language is shell-sensitive across platformsPrefer -D language=${lang} (or -D "language=${lang}") over single quotes to avoid issues on Windows.
- -D language='${lang}' + -D language=${lang}
156-157: Doctrees directory shared across languages may cause cache collisionsSphinx may reuse doctrees across language runs; separate per language to avoid stale translations.
- -d ${CMAKE_CURRENT_BINARY_DIR}/doctrees + -d ${CMAKE_CURRENT_BINARY_DIR}/doctrees/${lang}
| [.github/workflow/*.yml] | ||
| indent_style = space | ||
| indent_size = 2 | ||
| end_of_line = lf | ||
| charset = utf-8 | ||
| trim_trailing_whitespace = true | ||
| insert_final_newline = true | ||
|
|
There was a problem hiding this comment.
Fix path: it should be .github/workflows, not .github/workflow.
As written, the override won’t apply to GitHub workflow files.
-[.github/workflow/*.yml]
+[.github/workflows/*.yml]
indent_style = space
indent_size = 2
end_of_line = lf
charset = utf-8
trim_trailing_whitespace = true
insert_final_newline = true📝 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.
| [.github/workflow/*.yml] | |
| indent_style = space | |
| indent_size = 2 | |
| end_of_line = lf | |
| charset = utf-8 | |
| trim_trailing_whitespace = true | |
| insert_final_newline = true | |
| [.github/workflows/*.yml] | |
| indent_style = space | |
| indent_size = 2 | |
| end_of_line = lf | |
| charset = utf-8 | |
| trim_trailing_whitespace = true | |
| insert_final_newline = true |
🤖 Prompt for AI Agents
In .editorconfig around lines 34 to 41, the glob path is incorrect ("
.github/workflow/*.yml"); change it to ".github/workflows/*.yml" so the override
applies to GitHub Actions workflow files—update the section header text to the
correct directory name and save the file.
| for file in locale/en/LC_MESSAGES/*.po; do | ||
| if grep -q '#~' "$file"; then | ||
| perl -pi -0777 -e 's/#~.*//s' "$file" | ||
| git add "$file" | ||
| fi | ||
| done |
There was a problem hiding this comment.
Fix the overly aggressive regex pattern.
The current Perl regex s/#~.*//s will remove everything from the first #~ line to the end of the file, potentially removing valid content that comes after obsolete entries.
- perl -pi -0777 -e 's/#~.*//s' "$file"
+ perl -pi -0777 -e 's/^#~.*?(?=\n(?!#~)|$)//gm' "$file"This pattern removes only complete obsolete entries (lines starting with #~) while preserving other content.
📝 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.
| for file in locale/en/LC_MESSAGES/*.po; do | |
| if grep -q '#~' "$file"; then | |
| perl -pi -0777 -e 's/#~.*//s' "$file" | |
| git add "$file" | |
| fi | |
| done | |
| for file in locale/en/LC_MESSAGES/*.po; do | |
| if grep -q '#~' "$file"; then | |
| perl -pi -0777 -e 's/^#~.*?(?=\n(?!#~)|$)//gm' "$file" | |
| git add "$file" | |
| fi | |
| done |
🤖 Prompt for AI Agents
In .github/scripts/remove_obsolete_entries.sh around lines 10 to 15, the Perl
regex s/#~.*//s is overly broad and removes everything from the first "#~" to
EOF; change it to only delete lines (and any consecutive lines) that begin with
"#~" so other content is preserved — update the Perl invocation to use a
multiline-aware pattern (anchoring to line start, using the m and g modifiers)
that removes only lines starting with "#~" (and their consecutive "#~"
continuation lines) rather than everything after the first match.
| pushd "${DIR}" > /dev/null || exit | ||
| missing=$(! { mylicensecheck src & mylicensecheck sql & mylicensecheck include;} | grep "No copyright\|UNKNOWN") | ||
| missing1=$(mylicensecheck doc | grep "No copyright") | ||
| missing2=$(grep --files-without-match 'Creative Commons' doc/*/*.rst) | ||
| popd > /dev/null || exit |
There was a problem hiding this comment.
Fix races and grep alternation; broaden CC scan.
- Remove backgrounding to avoid data races (or add wait); use grep -E for alternation.
- Use find+xargs -0 to reliably scan all .rst files under doc, including nested dirs.
pushd "${DIR}" > /dev/null || exit
-missing=$(! { mylicensecheck src & mylicensecheck sql & mylicensecheck include;} | grep "No copyright\|UNKNOWN")
-missing1=$(mylicensecheck doc | grep "No copyright")
-missing2=$(grep --files-without-match 'Creative Commons' doc/*/*.rst)
+missing=$({ mylicensecheck src; mylicensecheck sql; mylicensecheck include; } \
+ | grep -E 'No copyright|UNKNOWN' || true)
+missing1=$(mylicensecheck doc | grep -F 'No copyright' || true)
+# Find all .rst files (excluding _build if present) missing the CC string
+missing2=$(find doc -type f -name '*.rst' -not -path '*/_build/*' -print0 \
+ | xargs -0 grep -L 'Creative Commons' || true)
popd > /dev/null || exitAlso consider quoting in conditions:
-if [[ $missing ]]; then
+if [[ -n "$missing" ]]; then
...
-if [[ $missing1 ]]; then
+if [[ -n "$missing1" ]]; then
...
-if [[ $missing2 ]]; then
+if [[ -n "$missing2" ]]; thenCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .github/scripts/test_license.sh around lines 13 to 17, the pipeline currently
backgrounded three mylicensecheck calls causing race conditions, uses basic grep
alternation, and only greps top-level doc files; remove the trailing ampersands
so commands run sequentially (or if parallelism is desired, capture PIDs and
wait), switch to grep -E "No copyright|UNKNOWN" for alternation, and replace the
simple grep over doc/*/*.rst with a find ... -name '*.rst' piped to xargs -0
grep --files-without-match 'Creative Commons' to recursively and safely scan
nested .rst files (also ensure command substitutions and test conditions are
properly quoted).
| run: | | ||
| sudo apt-get purge postgresql-* | ||
| sudo sh -c 'echo "deb http://apt.postgresql.org/pub/repos/apt $(lsb_release -cs)-pgdg-testing main ${{ matrix.psql }}" > /etc/apt/sources.list.d/pgdg.list' | ||
| curl https://www.postgresql.org/media/keys/ACCC4CF8.asc | gpg --dearmor | sudo tee /etc/apt/trusted.gpg.d/apt.postgresql.org.gpg >/dev/null | ||
|
|
There was a problem hiding this comment.
Remove undefined matrix variable (actionlint error).
The template references ${{ matrix.psql }}, which isn’t defined and triggers actionlint. Safe to drop.
- sudo sh -c 'echo "deb http://apt.postgresql.org/pub/repos/apt $(lsb_release -cs)-pgdg-testing main ${{ matrix.psql }}" > /etc/apt/sources.list.d/pgdg.list'
+ sudo sh -c 'echo "deb http://apt.postgresql.org/pub/repos/apt $(lsb_release -cs)-pgdg-testing main" > /etc/apt/sources.list.d/pgdg.list'📝 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.
| run: | | |
| sudo apt-get purge postgresql-* | |
| sudo sh -c 'echo "deb http://apt.postgresql.org/pub/repos/apt $(lsb_release -cs)-pgdg-testing main ${{ matrix.psql }}" > /etc/apt/sources.list.d/pgdg.list' | |
| curl https://www.postgresql.org/media/keys/ACCC4CF8.asc | gpg --dearmor | sudo tee /etc/apt/trusted.gpg.d/apt.postgresql.org.gpg >/dev/null | |
| run: | | |
| sudo apt-get purge postgresql-* | |
| sudo sh -c 'echo "deb http://apt.postgresql.org/pub/repos/apt $(lsb_release -cs)-pgdg-testing main" > /etc/apt/sources.list.d/pgdg.list' | |
| curl https://www.postgresql.org/media/keys/ACCC4CF8.asc | gpg --dearmor | sudo tee /etc/apt/trusted.gpg.d/apt.postgresql.org.gpg >/dev/null |
🧰 Tools
🪛 actionlint (1.7.7)
74-74: property "psql" is not defined in object type {release: string}
(expression)
🤖 Prompt for AI Agents
.github/workflows/clang.yml around lines 74 to 78: the workflow references an
undefined matrix variable ${{ matrix.psql }} in the apt source line which
triggers actionlint; remove that variable from the apt source entry so the
distro codename is used alone (e.g., "$(lsb_release -cs)-pgdg-testing main") and
ensure the templated string no longer contains any undefined matrix
interpolation.
| - name: Install dependencies | ||
| run: | | ||
| brew install postgresql@14 asio glpk rapidjson | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add gcc to Homebrew deps if you intend to build with GCC.
Currently you export gcc-12/g++-12 but don’t install GCC.
- brew install postgresql@14 asio glpk rapidjson
+ brew install postgresql@14 asio glpk rapidjson gcc📝 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.
| - name: Install dependencies | |
| run: | | |
| brew install postgresql@14 asio glpk rapidjson | |
| - name: Install dependencies | |
| run: | | |
| brew install postgresql@14 asio glpk rapidjson gcc |
🤖 Prompt for AI Agents
.github/workflows/macos.yml around lines 44 to 47: the workflow installs
Homebrew packages but omits installing GCC while later steps export
gcc-12/g++-12; add the appropriate Homebrew gcc package (e.g. gcc or gcc@12) to
the install list so the exported gcc-12/g++-12 binaries actually exist on the
runner; ensure the package name/version matches the gcc-12 export (use gcc@12 if
you need that exact version) and adjust any export paths if Homebrew installs a
different prefix.
| /* | ||
| * Instead of including all the files | ||
| * copy/paste what is used | ||
| * IMPORTANT: this copy/paste might depend on the postgreSQL version | ||
| */ |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid copy/paste approach - use proper includes instead.
The comment indicates that code is copy/pasted from PostgreSQL headers instead of including them properly. This creates significant maintenance risks:
- Version compatibility issues when PostgreSQL updates
- Potential for subtle bugs if definitions change
- Harder to track dependencies
Consider using proper PostgreSQL development headers or creating a more maintainable abstraction layer.
Let me verify if there are PostgreSQL development headers available in the project:
🏁 Script executed:
#!/bin/bash
# Check for PostgreSQL-related CMake configurations or include paths
fd -e cmake -e txt | xargs rg -l "PostgreSQL|postgres" -A 3Length of output: 102
I’ll check if the project already discovers and uses PostgreSQL headers via CMake:
🏁 Script executed:
#!/bin/bash
# Search for PostgreSQL package discovery and include_directories in all CMakeLists.txt
for file in $(fd CMakeLists.txt); do
echo "=== $file ==="
rg -n "find_package.*PostgreSQL" -A3 "$file" || true
rg -n "include_directories.*postgres" -i -A3 "$file" || true
doneLength of output: 7406
Replace copy-pasted PostgreSQL interrupt logic with upstream headers
The project already calls find_package(PostgreSQL) and adds ${PostgreSQL_INCLUDE_DIRS} in CMake, so you can—and should—drop the handwritten copy/paste of PostgreSQL macros and types in include/cpp_common/interruption.hpp.
• File include/cpp_common/interruption.hpp (lines 46–50, and the following block through line 78):
– Remove the “copy/paste what is used” comment and all inline defines (PGDLLIMPORT, unlikely(), INTERRUPTS_PENDING_CONDITION(), CHECK_FOR_INTERRUPTS(), etc.).
– Add the upstream headers that declare these symbols, for example:
c #include <postgres_ext.h> /* PGDLLIMPORT, etc. */ #include <utils/interrupt.h> /* CHECK_FOR_INTERRUPTS */ #include <port/pg_sema.h> /* SIGNAL HANDLING on Win32 */
• Windows handling: rely on the headers under ${PostgreSQL_INCLUDE_DIRS}/port/win32[_msvc] rather than re-implementing WIN32 guards.
• Macro syntax: you’ll no longer need to patch the missing backslash at line 77 once this block is removed.
This will eliminate version skew risk, reduce maintenance burden, and defer to PostgreSQL’s tested definitions.
🤖 Prompt for AI Agents
In include/cpp_common/interruption.hpp around lines 46 to 78, remove the
copy/pasted PostgreSQL macros/comment block (the “copy/paste what is used”
comment and all inline defines like PGDLLIMPORT, unlikely(),
INTERRUPTS_PENDING_CONDITION(), CHECK_FOR_INTERRUPTS(), etc.) and replace it by
including the upstream PostgreSQL headers that declare these symbols (for
example #include <postgres_ext.h>, #include <utils/interrupt.h>, #include
<port/pg_sema.h>), rely on the configured PostgreSQL include dirs for Windows
port headers instead of custom WIN32 guards, and delete the manual backslash fix
inserted at line 77 since it will no longer be needed.
| /* | ||
| * https://doxygen.postgresql.org/c_8h.html#a166c1d950e659804f0e3247aad99a81f | ||
| */ | ||
| #define PGDLLIMPORT |
There was a problem hiding this comment.
PGDLLIMPORT definition may be incorrect for Windows.
Defining PGDLLIMPORT as empty might cause issues on Windows when building shared libraries. PostgreSQL typically defines this as __declspec(dllimport) on Windows.
Consider using the proper platform-specific definition:
-#define PGDLLIMPORT
+#ifdef WIN32
+#define PGDLLIMPORT __declspec(dllimport)
+#else
+#define PGDLLIMPORT
+#endif📝 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.
| #define PGDLLIMPORT | |
| #ifdef WIN32 | |
| #define PGDLLIMPORT __declspec(dllimport) | |
| #else | |
| #define PGDLLIMPORT | |
| #endif |
🤖 Prompt for AI Agents
In include/cpp_common/interruption.hpp around line 56, PGDLLIMPORT is currently
defined as empty which is incorrect for Windows; replace it with a
platform-aware macro: under _WIN32 define PGDLLIMPORT as __declspec(dllimport)
(and __declspec(dllexport) when building the DLL — e.g., when a build symbol
like PGDLL_EXPORTS or similar is set), otherwise leave it empty on non-Windows
platforms; implement this with a simple #ifdef _WIN32 / #ifdef PGDLL_EXPORTS
conditional so Windows builds get the proper declspec while other platforms
remain unchanged.
| #ifndef WIN32 | ||
| #define INTERRUPTS_PENDING_CONDITION() (unlikely(InterruptPending)) | ||
| #else | ||
| extern void pgwin32_dispatch_queued_signals(void); | ||
| #define INTERRUPTS_PENDING_CONDITION() \ | ||
| (unlikely(UNBLOCKED_SIGNAL_QUEUE()) ? pgwin32_dispatch_queued_signals() : 0, \ | ||
| unlikely(InterruptPending)) | ||
| #endif |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Conflicting platform checks in non-MSVC block.
This code checks for WIN32 inside the #else block of #ifdef _MSC_VER (line 45), which creates confusion. Windows can be built with compilers other than MSVC (e.g., MinGW), so this logic is actually correct but could be clearer.
Additionally, line 77 appears to have a syntax issue with the comma operator usage.
Consider restructuring for clarity and fixing the syntax:
-#ifndef WIN32
-#define INTERRUPTS_PENDING_CONDITION() (unlikely(InterruptPending))
-#else
-extern void pgwin32_dispatch_queued_signals(void);
-#define INTERRUPTS_PENDING_CONDITION() \
- (unlikely(UNBLOCKED_SIGNAL_QUEUE()) ? pgwin32_dispatch_queued_signals() : 0, \
- unlikely(InterruptPending))
-#endif
+#ifdef WIN32
+extern void pgwin32_dispatch_queued_signals(void);
+#define INTERRUPTS_PENDING_CONDITION() \
+ (unlikely(UNBLOCKED_SIGNAL_QUEUE()) ? (pgwin32_dispatch_queued_signals(), 0) : 0, \
+ unlikely(InterruptPending))
+#else
+#define INTERRUPTS_PENDING_CONDITION() (unlikely(InterruptPending))
+#endifAlso, UNBLOCKED_SIGNAL_QUEUE() is referenced but not defined in this file.
Let me check if UNBLOCKED_SIGNAL_QUEUE is defined elsewhere:
🏁 Script executed:
#!/bin/bash
# Search for UNBLOCKED_SIGNAL_QUEUE definition
rg "UNBLOCKED_SIGNAL_QUEUE" --type hpp --type hLength of output: 75
I couldn’t find any definitions with the .hpp/.h filter—let’s broaden the search across the repo to confirm whether UNBLOCKED_SIGNAL_QUEUE is defined anywhere:
🏁 Script executed:
#!/bin/bash
# Search for UNBLOCKED_SIGNAL_QUEUE definition across all files
rg -n "UNBLOCKED_SIGNAL_QUEUE" .Length of output: 154
Clarify Windows guard and fix INTERRUPTS_PENDING_CONDITION syntax
The current nested checks use WIN32 only inside the non-MSVC branch, which misclassifies Windows builds under other compilers (e.g. MinGW defines _WIN32, not WIN32). Also, the ternary arm
unlikely(UNBLOCKED_SIGNAL_QUEUE()) ? pgwin32_dispatch_queued_signals() : 0mixes a void call with an int, and UNBLOCKED_SIGNAL_QUEUE() isn’t defined anywhere in the repo.
Please update include/cpp_common/interruption.hpp around lines 71–78 as follows:
• Use _WIN32 (the standard Windows macro) for the guard
• Ensure the dispatch-call branch yields an int (e.g. (pgwin32_dispatch_queued_signals(), 0))
• Include or define UNBLOCKED_SIGNAL_QUEUE() before using it
Suggested diff:
-#ifndef WIN32
-#define INTERRUPTS_PENDING_CONDITION() (unlikely(InterruptPending))
-#else
-extern void pgwin32_dispatch_queued_signals(void);
-#define INTERRUPTS_PENDING_CONDITION() \
- (unlikely(UNBLOCKED_SIGNAL_QUEUE()) ? pgwin32_dispatch_queued_signals() : 0, \
- unlikely(InterruptPending))
-#endif
+#if defined(_WIN32)
+ extern void pgwin32_dispatch_queued_signals(void);
+
+ /* Ensure this is defined in a shared header or add its definition */
+ #ifndef UNBLOCKED_SIGNAL_QUEUE
+ # error "UNBLOCKED_SIGNAL_QUEUE() is undefined — include its declaration"
+ #endif
+
+ #define INTERRUPTS_PENDING_CONDITION() \
+ ((unlikely(UNBLOCKED_SIGNAL_QUEUE()) \
+ ? (pgwin32_dispatch_queued_signals(), 0) \
+ : 0), \
+ unlikely(InterruptPending))
+#else
+ #define INTERRUPTS_PENDING_CONDITION() (unlikely(InterruptPending))
+#endif- Replace
WIN32with_WIN32to catch all Windows compilers. - Wrap the dispatch call in a comma‐expression that returns
0so both ternary arms areint. - Add or include the missing
UNBLOCKED_SIGNAL_QUEUE()definition.
This will ensure correct behavior on all Windows toolchains and eliminate the type‐mismatch and undefined‐macro errors.
🤖 Prompt for AI Agents
In include/cpp_common/interruption.hpp around lines 71 to 78, the Windows guard
and INTERRUPTS_PENDING_CONDITION macro are incorrect: switch the platform check
to _WIN32 (the standard Windows macro), ensure the dispatch-call branch produces
an int by using a comma-expression so the ternary returns an int (e.g. call the
void function then yield 0), and add or include a definition for
UNBLOCKED_SIGNAL_QUEUE() before this use (either include the header that defines
it or add a small macro wrapper that evaluates the signal-queue condition).
Ensure the resulting macro consistently evaluates to an int expression on
Windows and non-Windows builds.
| Messages() = default; | ||
| Messages(const Messages&) {} | ||
| Messages& operator=(const Messages&) {return *this;} | ||
|
|
There was a problem hiding this comment.
Fix broken copy/assign semantics (lossy copies) for Messages
Current copy ctor/assignment are no-ops, silently discarding content. Streams are non-copyable; either delete copy or implement move semantics. Safer to delete copy and allow move.
public:
- Messages() = default;
- Messages(const Messages&) {}
- Messages& operator=(const Messages&) {return *this;}
+ Messages() = default;
+ Messages(const Messages&) = delete;
+ Messages& operator=(const Messages&) = delete;
+#if __cplusplus >= 201103L
+ Messages(Messages&&) noexcept = default;
+ Messages& operator=(Messages&&) noexcept = default;
+#endifAlternatively, if you need copy-by-content, implement:
- log.str(other.log.str()); log.clear();
- notice.str(other.notice.str()); notice.clear();
- error.str(other.error.str()); error.clear();
…but be explicit, not silent no-ops.
📝 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.
| Messages() = default; | |
| Messages(const Messages&) {} | |
| Messages& operator=(const Messages&) {return *this;} | |
| --- a/include/cpp_common/messages.hpp | |
| +++ b/include/cpp_common/messages.hpp | |
| @@ public: | |
| - Messages() = default; | |
| - Messages(const Messages&) {} | |
| - Messages& operator=(const Messages&) {return *this;} | |
| + Messages() = default; | |
| + Messages(const Messages&) = delete; | |
| + Messages& operator=(const Messages&) = delete; | |
| +#if __cplusplus >= 201103L | |
| + Messages(Messages&&) noexcept = default; | |
| + Messages& operator=(Messages&&) noexcept = default; | |
| +#endif |
🤖 Prompt for AI Agents
In include/cpp_common/messages.hpp around lines 54–57, the copy constructor and
copy assignment are no-op stubs that silently discard content; replace these
with safe semantics by deleting the copy operations and providing move semantics
instead: remove the empty Messages(const Messages&) and Messages&
operator=(const Messages&), add =delete for both copy ctor and copy assignment,
and declare/define a defaulted move constructor and move assignment
(Messages(Messages&&) noexcept = default; Messages& operator=(Messages&&)
noexcept = default;) so streams are transferred rather than silently lost;
alternatively, if true copy-by-content is required, implement deep copy using
the stringstream::str() contents for each member (e.g.,
dst.log.str(src.log.str()); dst.log.clear(); etc.), but be explicit rather than
leaving silent no-ops.
| #include "cpp_common/undefPostgresDefine.hpp" | ||
|
|
There was a problem hiding this comment.
Ensure Idx is defined: include c_types/typedefs.h explicitly
The signature uses Idx in a public return type. Relying on transitive includes is brittle and may fail to compile.
#include <map>
#include "cpp_common/undefPostgresDefine.hpp"
+#include "c_types/typedefs.h"🤖 Prompt for AI Agents
In include/cpp_common/pgdata_getters.hpp around lines 45 to 46, the header
relies on a transitive include for the Idx typedef used in a public return type;
explicitly add an #include "c_types/typedefs.h" (placed before any headers that
might undef/override Postgres defines or at the top of the file) so Idx is
always defined and the header compiles independently.
| for f in $(git ls-files | grep '\.sh') | ||
| do | ||
| result=$(shellcheck "${f}") | ||
|
|
||
| if [[ $result ]]; then | ||
| echo "$result" | ||
| echo " *** shellcheck found script errors while processing $f" | ||
| code=1 | ||
| fi | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider safer iteration over filenames.
The current for loop could fail with filenames containing spaces or special characters. Consider using a more robust approach.
-for f in $(git ls-files | grep '\.sh')
-do
- result=$(shellcheck "${f}")
-
- if [[ $result ]]; then
- echo "$result"
- echo " *** shellcheck found script errors while processing $f"
- code=1
- fi
-done
+while IFS= read -r -d '' f; do
+ result=$(shellcheck "${f}")
+
+ if [[ $result ]]; then
+ echo "$result"
+ echo " *** shellcheck found script errors while processing $f"
+ code=1
+ fi
+done < <(git ls-files -z '*.sh')🤖 Prompt for AI Agents
In .github/scripts/test_shell.sh around lines 11 to 20, the for loop using
command substitution can break on filenames with spaces or special characters;
replace it with a null-delimited safe iteration using git ls-files -z piped into
while IFS= read -r -d '' f; do ...; done, ensure you quote "$f" when calling
shellcheck and when printing, and preserve the existing logic for capturing
shellcheck output/exit status and setting the code variable.
| for s1 in "${SIGNATURES[@]}" | ||
| do | ||
| for s2 in "${SIGNATURES[@]}" | ||
| do | ||
| # only comparing lower version with higher version | ||
| if (( $(echo "$s1 >= $s2" | bc -l) )); then continue; fi | ||
|
|
||
| mayor1=$(echo "$s1" | perl -pe 's/([0-9]+).*/$1/') | ||
| mayor2=$(echo "$s2" | perl -pe 's/([0-9]+).*/$1/') | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Version comparison is broken; bc cannot parse dotted versions
You’re feeding “0.1.0” to bc, which is invalid. Use dpkg --compare-versions or sort -V and compare indices; also group by major (>0).
-for s1 in "${SIGNATURES[@]}"
-do
- for s2 in "${SIGNATURES[@]}"
- do
- # only comparing lower version with higher version
- if (( $(echo "$s1 >= $s2" | bc -l) )); then continue; fi
-
- mayor1=$(echo "$s1" | perl -pe 's/([0-9]+).*/$1/')
- mayor2=$(echo "$s2" | perl -pe 's/([0-9]+).*/$1/')
+for s1 in "${SIGNATURES[@]}"; do
+ for s2 in "${SIGNATURES[@]}"; do
+ # Compare only if s1 < s2 using version semantics
+ if dpkg --compare-versions "$s1" ge "$s2"; then continue; fi
+ mayor1=$(echo "$s1" | perl -pe 's/([0-9]+).*/$1/')
+ mayor2=$(echo "$s2" | perl -pe 's/([0-9]+).*/$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.
| for s1 in "${SIGNATURES[@]}" | |
| do | |
| for s2 in "${SIGNATURES[@]}" | |
| do | |
| # only comparing lower version with higher version | |
| if (( $(echo "$s1 >= $s2" | bc -l) )); then continue; fi | |
| mayor1=$(echo "$s1" | perl -pe 's/([0-9]+).*/$1/') | |
| mayor2=$(echo "$s2" | perl -pe 's/([0-9]+).*/$1/') | |
| for s1 in "${SIGNATURES[@]}"; do | |
| for s2 in "${SIGNATURES[@]}"; do | |
| # Compare only if s1 < s2 using version semantics | |
| if dpkg --compare-versions "$s1" ge "$s2"; then continue; fi | |
| mayor1=$(echo "$s1" | perl -pe 's/([0-9]+).*/$1/') | |
| mayor2=$(echo "$s2" | perl -pe 's/([0-9]+).*/$1/') |
🤖 Prompt for AI Agents
.github/scripts/test_signatures.sh around lines 12 to 21: the script currently
uses bc to compare dotted versions like "0.1.0" which bc cannot parse; replace
the bc numeric comparison with a proper semantic-version comparison (e.g., use
dpkg --compare-versions "$s1" lt "$s2" in the if-condition) or alternatively
build a sorted list with sort -V and compare the indices of s1 and s2; also
extract and compare major versions using cut -d. -f1 (or split on '.') to group
by major before doing finer comparisons so you only compare versions across the
same major when intended.
| # ignoring any signature changes made on v0 | ||
| if [ "$mayor1" == 0 ]; then continue; fi | ||
|
|
||
| missing+=$(diff "pgvroom--$s1.sig" "pgvroom--$s2.sig" | grep '<') | ||
| done | ||
| done | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Anchor removed-lines grep and preserve newlines
Use grep '^<' and append a newline to keep entries readable.
- missing+=$(diff "pgvroom--$s1.sig" "pgvroom--$s2.sig" | grep '<')
+ diff_out=$(diff "pgvroom--$s1.sig" "pgvroom--$s2.sig" | grep '^<' || true)
+ if [ -n "$diff_out" ]; then
+ missing+="${diff_out}"$'\n'
+ fi📝 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.
| # ignoring any signature changes made on v0 | |
| if [ "$mayor1" == 0 ]; then continue; fi | |
| missing+=$(diff "pgvroom--$s1.sig" "pgvroom--$s2.sig" | grep '<') | |
| done | |
| done | |
| # ignoring any signature changes made on v0 | |
| if [ "$mayor1" == 0 ]; then continue; fi | |
| diff_out=$(diff "pgvroom--$s1.sig" "pgvroom--$s2.sig" | grep '^<' || true) | |
| if [ -n "$diff_out" ]; then | |
| missing+="${diff_out}"$'\n' | |
| fi | |
| done | |
| done |
🤖 Prompt for AI Agents
.github/scripts/test_signatures.sh around lines 25 to 31: the diff selection
uses grep '<' which can match inside lines and loses newline separation when
concatenating into the missing variable; change the grep to anchor the beginning
of the line (grep '^<') so only removed lines are matched, and ensure each
matched line ends with a newline when appended to the missing variable (for
example by piping the grep output through a simple transformer that appends '\n'
to each line or by iterating over matches and appending each with a trailing
newline) so entries remain readable.
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
| - name: Signatures are not removed | ||
| run: ./test_signatures.sh | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validate referenced scripts and tooling paths
Given defaults.run.working-directory is .github/scripts, confirm these scripts exist and have execute bits; also ensure tools/scripts/code_checker.sh exists and can find cpplint in code_linter/cpplint.
Also applies to: 51-57, 61-67, 71-81
🏁 Script executed:
#!/bin/bash
set -e
echo "Checking scripts..."
for f in .github/scripts/test_signatures.sh \
.github/scripts/test_license.sh \
.github/scripts/test_shell.sh \
tools/scripts/code_checker.sh; do
if [[ -f "$f" ]]; then
echo "FOUND: $f"
[[ -x "$f" ]] || echo "WARN: not executable -> $f"
else
echo "MISSING: $f"
fi
done
echo "Checking cpplint checkout path..."
test -d code_linter/cpplint && echo "FOUND: code_linter/cpplint" || echo "MISSING: code_linter/cpplint"Length of output: 1471
Fix missing cpplint tooling path in CI workflow
We’ve verified that all .github/scripts/*.sh and tools/scripts/code_checker.sh exist and are executable. However, the required code_linter/cpplint directory is missing, so code_checker.sh won’t be able to locate cpplint.
• In .github/workflows/files.yml (also for the other code_checker.sh invocations at lines 51–57, 61–67 and 71–81), add a step to checkout or install cpplint into code_linter/cpplint before running the linter.
Example:
- name: Checkout cpplint
uses: actions/checkout@v4
with:
repository: “github.com/<owner>/cpplint”
path: code_linter/cpplint• Alternatively, update tools/scripts/code_checker.sh to point at the correct location where cpplint is already installed.
🤖 Prompt for AI Agents
In .github/workflows/files.yml around lines 31 to 35 (and similarly for the
other code_checker.sh invocations at 51–57, 61–67 and 71–81), the workflow runs
tools/scripts/code_checker.sh but never provides the code_linter/cpplint
directory so cpplint cannot be found; fix this by adding a step before each
code_checker.sh run that checks out or installs cpplint into code_linter/cpplint
(e.g., a checkout step using actions/checkout@v4 with repository set to the
cpplint repo and path set to code_linter/cpplint), or alternatively modify
tools/scripts/code_checker.sh to reference the actual installed path of cpplint
on the runner; ensure the cpplint checkout/install step executes before invoking
code_checker.sh for each occurrence.
| run: sudo apt install -y licensecheck | ||
| - name: Run License Check | ||
| run: ./test_license.sh |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add apt-get update before installs to avoid transient failures
Ubuntu runners sometimes require an update; installs may fail without it.
- - name: Install Requirements
- run: sudo apt install -y licensecheck
+ - name: Install Requirements
+ run: |
+ sudo apt-get update
+ sudo apt-get install -y licensecheck
@@
- - name: Install Requirements
- run: sudo apt-get install -y shellcheck
+ - name: Install Requirements
+ run: |
+ sudo apt-get update
+ sudo apt-get install -y shellcheckAlso applies to: 64-66
🤖 Prompt for AI Agents
.github/workflows/files.yml around lines 54 to 56 (and similarly lines 64-66):
the workflow installs packages directly which can fail on Ubuntu runners if the
package lists are stale; add a step to run sudo apt-get update (or prefix
installs with sudo apt-get update -y && ) before any sudo apt install commands
so package lists are refreshed and installs become reliable; apply the same
change to the other install block at lines 64-66.
| .. admonition:: \ \ | ||
| :class: signatures | ||
|
|
||
| | vro_vroom( |
There was a problem hiding this comment.
Fix incorrect function name in signature
The function signature shows vro_vroom but this documentation is for vro_vroomJobsPlain.
- | vro_vroom(
+ | vro_vroomJobsPlain(📝 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.
| | vro_vroom( | |
| | vro_vroomJobsPlain( |
🤖 Prompt for AI Agents
In doc/vroom/vro_vroomJobsPlain.rst around line 40, the function signature
incorrectly shows `vro_vroom` instead of `vro_vroomJobsPlain`; update the
signature to use the correct function name `vro_vroomJobsPlain` so the
documentation matches the actual API, and verify any cross-references or
examples nearby also use the corrected name.
| single: vro_vroomPlain -- Experimental on v0.2 | ||
|
|
There was a problem hiding this comment.
Version inconsistency in experimental marking
The documentation marks this as "Experimental on v0.2" but the PR objectives state this is for v0.1.0. Should this be "Experimental on v0.1"?
Also applies to: 48-48
🤖 Prompt for AI Agents
In doc/vroom/vro_vroomPlain.rst around lines 34-35 and line 48, the text marks
the feature as "Experimental on v0.2" but the PR targets v0.1; update both
occurrences to "Experimental on v0.1" (or "v0.1.0" to match project versioning)
so the documentation matches the PR objectives and release target.
| .. admonition:: \ \ | ||
| :class: signatures | ||
|
|
||
| | vro_vroom( |
There was a problem hiding this comment.
Incorrect function name in signature
The signature shows vro_vroom but this documentation is for vro_vroomPlain.
- | vro_vroom(
+ | vro_vroomPlain(📝 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.
| | vro_vroom( | |
| | vro_vroomPlain( |
🤖 Prompt for AI Agents
In doc/vroom/vro_vroomPlain.rst around line 42, the function signature
incorrectly uses `vro_vroom`; update the signature to `vro_vroomPlain` to match
the documented function name and ensure consistency with the rest of the file
and API docs.
| SELECT pgr_version(); | ||
| pgr_version | ||
| ------------- | ||
| 3.7.0-dev | ||
| (1 row) |
There was a problem hiding this comment.
Switch to vro_version() and align expected output to v0.1.0
This fixture still targets pgr_version and a pgRouting dev version. Update to vro_version and the project’s v0.1.0.
-SELECT pgr_version();
- pgr_version
+SELECT vro_version();
+ vro_version
-------------
- 3.7.0-dev
+ 0.1.0
(1 row)📝 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.
| SELECT pgr_version(); | |
| pgr_version | |
| ------------- | |
| 3.7.0-dev | |
| (1 row) | |
| SELECT vro_version(); | |
| vro_version | |
| ------------- | |
| 0.1.0 | |
| (1 row) |
🤖 Prompt for AI Agents
In docqueries/version.result around lines 6 to 10, the fixture is calling
pgr_version() and expecting a pgRouting dev version; change the SQL to call
vro_version() and update the expected output row to v0.1.0 so the file reflects
the project version. Ensure the output formatting stays consistent with the
existing fixture (single-column header matching vro_version and a single row
showing v0.1.0).
| SELECT * | ||
| FROM vro_vroomJobsPlain( | ||
| 'SELECT * FROM plain.jobs', | ||
| 'SELECT * FROM plain.jobs_time_windows', | ||
| 'SELECT * FROM plain.vehicles', | ||
| 'SELECT * FROM plain.breaks', | ||
| 'SELECT * FROM plain.breaks_time_windows', | ||
| 'SELECT * FROM plain.matrix' | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Stabilize results with ORDER BY; remove unused q3 marker
Doc outputs are easier to diff and validate if deterministic. Add ORDER BY to both queries and drop the unused q3 marker.
SELECT *
FROM vro_vroomJobsPlain(
@@
-);
+)
+ORDER BY 1;
@@
SELECT *
FROM vro_vroomJobsPlain(
@@
-);
+)
+ORDER BY 1;
-/* -- q3 */If you have known output columns (e.g., vehicle_id, step_idx), prefer an explicit ORDER BY on those instead of ORDER BY 1.
Also applies to: 12-35, 36-36
🏁 Script executed:
#!/bin/bash
# Display the first 100 lines to inspect SELECT blocks
sed -n '1,100p' docqueries/vroom/vroomJobsPlain.pg
# Search for any stray q3 markers in the file
grep -n 'q3' docqueries/vroom/vroomJobsPlain.pg || echo "No 'q3' occurrences found"Length of output: 964
Add deterministic ORDER BY and remove stray q3 marker
To make generated docs diff-friendly and stable, sort each result set and drop the unused q3 placeholder:
• File: docqueries/vroom/vroomJobsPlain.pg
– After both SELECT * FROM vro_vroomJobsPlain(...) blocks, append ORDER BY 1; (or explicit columns if known).
– Remove the /* -- q3 */ line at the end.
Suggested diff:
--- a/docqueries/vroom/vroomJobsPlain.pg
+++ b/docqueries/vroom/vroomJobsPlain.pg
@@ /* -- q1 */
-);
+)
+ORDER BY 1;
@@ /* -- q2 */
-);
+)
+ORDER BY 1;
@@ -36,1 +36,0
-/* -- q3 */If you know the actual output columns (e.g. vehicle_id, step_idx), prefer ORDER BY vehicle_id, step_idx over ORDER BY 1.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In docqueries/vroom/vroomJobsPlain.pg around lines 2 to 10, the query results
are non-deterministic and a stray comment marker `/* -- q3 */` remains; to fix,
append a deterministic ORDER BY clause to each SELECT result of
vro_vroomJobsPlain (use ORDER BY 1 if column names are unknown, or preferably
explicit columns like ORDER BY vehicle_id, step_idx) and remove the `/* -- q3
*/` line at the end so generated docs are stable and the placeholder is gone.
|
@coderabbitai Open an issue summarizing the problems found |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll analyze the codebase to identify the problems found and then create a GitHub issue summarizing them. Now I'll create a GitHub issue summarizing these problems: @cvvergara I've created a GitHub issue summarizing the code quality problems found in this PR. The issue covers several key areas including:
The issue has been assigned to you and includes checkboxes for tracking the fixes. Each problem is documented with its location, impact, and recommended solution. 🏁 Script executed: Length of output: 2828 🏁 Script executed: Length of output: 2105 🏁 Script executed: Length of output: 1463 🏁 Script executed: Length of output: 1920 @cvvergara I've successfully created GitHub Issue #3 summarizing the code quality problems found in this PR. Key Issues Identified:
Note: I initially flagged DoxygenLayout.xml as having a filename issue, but upon verification, it's correctly named. The issue has been assigned to you and includes actionable checkboxes for tracking fixes. Each problem is documented with its exact location, impact assessment, and recommended solution. |
3 participants
Fixes #1
Changes proposed in this pull request:
@pgRouting/admins
Summary by CodeRabbit