Skip to content

Conversation

@cvvergara
Copy link
Member

@cvvergara cvvergara commented Jan 5, 2026

This is a reorganization PR

pgr_topologicalSort is about ordering

  • src, include and sql are moved to ordering directory
    • Build in ordering directory is adjusted
  • docqqueries, doc and pgtap are already in the correct directory
  • Code is adjusted to new include file locations
  • define guards are adjusted to new locations
    • including a fix for path.hpp

No need to document, its just moving code around

@pgRouting/admins

Summary by CodeRabbit

  • Refactor
    • Reorganized topological sort components under a unified module structure for improved codebase organization and maintainability.

✏️ Tip: You can customize this high-level summary in your review settings.

@cvvergara cvvergara added this to the Release 4.1.0 milestone Jan 5, 2026
@cvvergara cvvergara added the Reorganization Move code around label Jan 5, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 5, 2026

Walkthrough

The changes reorganize the topologicalSort feature from a standalone module into the ordering module, including moving source files, headers, and SQL scripts while updating include paths and build configurations to reflect the new directory structure.

Changes

Cohort / File(s) Summary
Configuration Removal
configuration.conf
Removed topologicalSort entry declaring C/C++, SQL, and documentation support.
Header Guard Renames
include/cpp_common/path.hpp, include/drivers/ordering/topologicalSort_driver.h, include/ordering/topologicalSort.hpp
Updated include guards to reflect new module paths: BASEPATH_SSEC → PATH, TOPOLOGICALSORT_TOPOLOGICALSORT → ORDERING_TOPOLOGICALSORT, and TOPOLOGICALSORT_TOPOLOGICALSORT → ORDERING_TOPOLOGICALSORT.
Build Reorganization
src/ordering/CMakeLists.txt, src/topologicalSort/CMakeLists.txt
Moved topologicalSort.c and topologicalSort_driver.cpp from standalone topologicalSort target into ordering OBJECT library; removed former target definition.
SQL File Relocation
sql/ordering/CMakeLists.txt, sql/topologicalSort/CMakeLists.txt
Migrated _topologicalSort.sql and topologicalSort.sql from standalone sql/topologicalSort to sql/ordering directory; removed legacy build steps.
Include Path Updates
src/ordering/topologicalSort.c, src/ordering/topologicalSort_driver.cpp
Updated include directives to reference relocated headers: drivers/ordering/topologicalSort_driver.h and ordering/topologicalSort.hpp.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Suggested reviewers

  • robe2
  • iosefa

Poem

🐰 A hop and a skip through the folder tree,
Topological sorts now organized, you see!
From scattered subdirs to ordering's home,
Our features consolidated—no more to roam!
whisker-twitch of satisfaction

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Move pgr_topologicalSort to ordering' accurately and concisely describes the main objective of the pull request—a repository reorganization that relocates the topologicalSort implementation from a standalone directory to an ordering subdirectory.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7b7c422 and 4f8745d.

📒 Files selected for processing (12)
  • configuration.conf
  • include/cpp_common/path.hpp
  • include/drivers/ordering/topologicalSort_driver.h
  • include/ordering/topologicalSort.hpp
  • sql/ordering/CMakeLists.txt
  • sql/ordering/_topologicalSort.sql
  • sql/ordering/topologicalSort.sql
  • sql/topologicalSort/CMakeLists.txt
  • src/ordering/CMakeLists.txt
  • src/ordering/topologicalSort.c
  • src/ordering/topologicalSort_driver.cpp
  • src/topologicalSort/CMakeLists.txt
💤 Files with no reviewable changes (3)
  • sql/topologicalSort/CMakeLists.txt
  • src/topologicalSort/CMakeLists.txt
  • configuration.conf
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 2771
File: doc/topology/pgr_extractVertices.rst:28-35
Timestamp: 2025-02-27T23:09:12.162Z
Learning: In pgRouting documentation, historical version entries should be maintained in reverse chronological order (newest first) to provide context about the evolution of functions over time.
📚 Learning: 2025-04-24T23:29:43.790Z
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 2868
File: tools/testers/contractionHierarchies_data.sql:73-85
Timestamp: 2025-04-24T23:29:43.790Z
Learning: Test data for pgRouting algorithms like contraction hierarchies doesn't necessarily need to follow all production best practices such as setting explicit SRIDs on geometries, especially when the algorithm primarily works with graph topology rather than geographic properties.

Applied to files:

  • include/cpp_common/path.hpp
📚 Learning: 2025-04-25T00:59:52.580Z
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 2868
File: src/contraction/contractionHierarchies_driver.cpp:47-56
Timestamp: 2025-04-25T00:59:52.580Z
Learning: In pgRouting, driver functions like `pgr_contractionHierarchies` assume that callers within the project will pass valid non-null pointers, as these functions are only used internally with well-defined call patterns. Null checks on the pointers themselves (as opposed to their dereferenced values) are considered unnecessary since the caller is guaranteed to pass references to variables.

Applied to files:

  • include/cpp_common/path.hpp
📚 Learning: 2025-04-25T02:12:17.271Z
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 2871
File: include/drivers/transitiveClosure/transitiveClosure_driver.h:45-49
Timestamp: 2025-04-25T02:12:17.271Z
Learning: The pgRouting project avoids using named parameters in C header files to prevent conflicts with Doxygen documentation.

Applied to files:

  • include/cpp_common/path.hpp
📚 Learning: 2025-02-27T16:33:56.959Z
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 2764
File: sql/sigs/pgrouting--3.8.sig:125-136
Timestamp: 2025-02-27T16:33:56.959Z
Learning: The file sql/sigs/pgrouting--*.sig is automatically generated by a command and cannot be manually modified. Comments about naming conventions or other improvements should be directed at the source code that generates these signatures, not at the signature file itself.

Applied to files:

  • include/cpp_common/path.hpp
📚 Learning: 2025-02-27T16:34:57.518Z
Learnt from: cvvergara
Repo: pgRouting/pgrouting PR: 2764
File: sql/sigs/pgrouting--3.8.sig:274-275
Timestamp: 2025-02-27T16:34:57.518Z
Learning: The file `sql/sigs/pgrouting--3.8.sig` is auto-generated by a command and should not be modified manually.

Applied to files:

  • sql/ordering/CMakeLists.txt
🪛 Cppcheck (2.19.0)
src/ordering/topologicalSort.c

[information] 38-38: Include file

(missingIncludeSystem)


[information] 38-38: Include file

(missingIncludeSystem)

src/ordering/topologicalSort_driver.cpp

[information] 29-29: Include file

(missingIncludeSystem)


[information] 38-38: Include file

(missingIncludeSystem)


[information] 38-38: Include file

(missingIncludeSystem)


[information] 38-38: Include file

(missingIncludeSystem)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Build (56, 4.0.0)
  • GitHub Check: Build (56, 3.6.3)
  • GitHub Check: macos (macos-14)
  • GitHub Check: macos (macos-latest)
  • GitHub Check: Ubuntu Boost (78)
  • GitHub Check: Ubuntu clang (Debug, ubuntu-latest)
  • GitHub Check: Ubuntu Boost (75)
  • GitHub Check: Ubuntu Boost (83)
  • GitHub Check: Ubuntu Boost (80)
  • GitHub Check: Ubuntu clang (Release, ubuntu-22.04)
  • GitHub Check: Ubuntu Boost (86)
  • GitHub Check: Ubuntu Boost (68)
  • GitHub Check: Ubuntu Boost (76)
  • GitHub Check: Ubuntu Boost (84)
  • GitHub Check: Ubuntu clang (Debug, ubuntu-22.04)
  • GitHub Check: Ubuntu Boost (56)
  • GitHub Check: Ubuntu clang (Release, ubuntu-latest)
  • GitHub Check: Ubuntu Boost (77)
  • GitHub Check: Ubuntu Boost (79)
  • GitHub Check: Build
🔇 Additional comments (7)
sql/ordering/CMakeLists.txt (1)

8-9: LGTM! SQL files correctly added to build configuration.

The topologicalSort SQL files are properly added to the LOCAL_FILES list, consistent with the existing pattern for other ordering functions.

include/cpp_common/path.hpp (1)

30-31: LGTM! Include guard corrected to match filename.

The guard has been updated from BASEPATH_SSEC to PATH, which properly reflects the actual filename path.hpp. This is a beneficial correction.

Also applies to: 349-349

include/ordering/topologicalSort.hpp (1)

25-26: LGTM! Include guard updated for new location.

The guard correctly reflects the file's new location in the ordering/ directory, following the project's naming convention.

Also applies to: 101-101

src/ordering/topologicalSort_driver.cpp (1)

29-29: LGTM! Include paths correctly updated for reorganization.

Both the driver header (line 29) and the main implementation header (line 38) now reference their new locations in the ordering/ directory structure.

Also applies to: 38-38

src/ordering/topologicalSort.c (1)

38-38: The include path change is correct and verified.

All references have been properly updated. No stale references to old topologicalSort paths exist in the codebase. The new path drivers/ordering/topologicalSort_driver.h is valid and all files using this header (including src/ordering/topologicalSort_driver.cpp) are correctly updated.

src/ordering/CMakeLists.txt (1)

6-7: TopologicalSort files properly integrated into ordering library. The source files have been correctly added to the ordering OBJECT library and the previous location and CMake target have been removed.

include/drivers/ordering/topologicalSort_driver.h (1)

31-32: Include guard correctly updated to reflect new path.

The include guard macro has been properly renamed to INCLUDE_DRIVERS_ORDERING_TOPOLOGICALSORT_DRIVER_H_, which correctly reflects the file's new location in include/drivers/ordering/. The endif comment is also correct. All source file includes have been updated consistently to use the new path.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cvvergara cvvergara requested a review from robe2 January 5, 2026 17:27
@cvvergara cvvergara merged commit a335a6c into pgRouting:develop Jan 6, 2026
59 checks passed
@cvvergara cvvergara deleted the move-topological-sort-to-ordering branch January 6, 2026 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Reorganization Move code around

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants