diff --git a/agents/vertex_id_const_ref_plan.md b/agents/vertex_id_const_ref_plan.md new file mode 100644 index 0000000..a652ed9 --- /dev/null +++ b/agents/vertex_id_const_ref_plan.md @@ -0,0 +1,675 @@ +# Implementation Plan: Pass `vertex_id_t` by `const&` + +Implements [vertex_id_const_ref_strategy.md](vertex_id_const_ref_strategy.md). + +## Safety Principles + +Every step in this plan follows these safety rules: + +0. **Work on a feature branch.** All changes are made on a dedicated branch + `feature/vertex-id-const-ref` created from the current `main` (or primary development branch). + Each phase gate is a natural commit point. The branch is merged only after final validation. + +1. **One logical change per step.** Each step modifies one category of files (e.g., one header, + or one group of tightly-coupled headers). Never mix changes across unrelated subsystems. + +2. **Build gate after every step.** No step is considered complete until it compiles cleanly on + at least `linux-gcc-debug` and `linux-clang-debug`. Steps involving reference semantics also + require `linux-gcc-asan`. + +3. **Test gate after every step.** Run the relevant targeted test suite after each step, plus a + full test suite run at the end of each phase. + +4. **No skipping ahead.** Each phase depends on the previous phase being fully green. Phase 1 + depends on Phase 0. Phase 2 depends on Phase 1. Etc. + +5. **Revert on red.** If a step breaks the build or tests, revert it entirely before diagnosing. + Do not attempt fixes on top of a broken state. + +6. **Static assertions as guardrails.** Add `static_assert` checks at key points to verify type + assumptions at compile time, especially for `vertex_id_store_t` resolution. + +7. **Preserve existing `using` aliases.** When changing algorithm internals, keep + `using id_type = ...` on its own line so the diff is minimal and reviewable. + +--- + +## Build & Test Commands + +```bash +# Create and switch to feature branch (do this once, before any changes): +git checkout -b feature/vertex-id-const-ref + +# Targeted build (one preset): +cmake --build build/linux-gcc-debug -j $(nproc) + +# Targeted test (one preset): +ctest --test-dir build/linux-gcc-debug --output-on-failure + +# ASan build + test: +cmake --build build/linux-gcc-asan -j $(nproc) +ctest --test-dir build/linux-gcc-asan --output-on-failure + +# Full matrix (all presets): +./scripts/test-all.sh +``` + +--- + +## Phase 0: Type Alias Foundation + +**Goal:** Add `raw_vertex_id_t` and `vertex_id_store_t` aliases; protect `vertex_id_t` +with `remove_cvref_t`. These are purely additive — no existing code changes behavior. + +### Step 0.1: Protect `vertex_id_t` in `graph_cpo.hpp` + +**File:** `include/graph/adj_list/detail/graph_cpo.hpp` + +**Change:** Wrap the `vertex_id_t` definition in `std::remove_cvref_t<>`. + +```cpp +// Before: +template +using vertex_id_t = decltype(vertex_id(std::declval(), std::declval>())); + +// After: +template +using vertex_id_t = std::remove_cvref_t< + decltype(vertex_id(std::declval(), std::declval>()))>; +``` + +**Add** the `raw_vertex_id_t` alias immediately below: +```cpp +template +using raw_vertex_id_t = decltype(vertex_id(std::declval(), std::declval>())); +``` + +**Validation:** +- Build: `linux-gcc-debug`, `linux-clang-debug` +- Test: `ctest --test-dir build/linux-gcc-debug --output-on-failure` +- Verify: `remove_cvref_t` on a value type is a no-op — all existing tests must pass unchanged. + +### Step 0.2: Protect `vertex_id_t` in `edge_list.hpp` + +**File:** `include/graph/edge_list/edge_list.hpp` + +**Change:** Apply the same `std::remove_cvref_t<>` wrapping to the edge_list `vertex_id_t` +definition, if not already wrapped. + +**Validation:** Build + test `linux-gcc-debug`. + +### Step 0.3: Add `vertex_id_store_t` alias + +**File:** `include/graph/algorithm/traversal_common.hpp` (already included by all algorithms) + +**Change:** Add the `vertex_id_store_t` alias, gated on `` and ``: +```cpp +/// Efficient storage type for vertex IDs in algorithm-internal containers. +/// For integral IDs: same as vertex_id_t (zero overhead). +/// For non-trivial IDs (e.g., string from map-based graphs): reference_wrapper +/// to the stable key in the graph's map node (8 bytes, trivially copyable). +/// Requires: the graph must not be mutated while values of this type are alive. +template +using vertex_id_store_t = std::conditional_t< + std::is_reference_v>, + std::reference_wrapper>>, + adj_list::vertex_id_t>; +``` + +**Validation:** Build + full test suite on `linux-gcc-debug`. + +### Step 0.4: Phase 0 gate — full matrix build and commit + +**Commands:** +```bash +./scripts/test-all.sh +git add -A && git commit -m "Phase 0: Add raw_vertex_id_t, vertex_id_store_t; protect vertex_id_t with remove_cvref_t" +``` + +**Exit criterion:** All four presets build and test green. No behavioral changes — this phase +is purely additive type aliases. + +--- + +## Phase 1: Descriptor Return Types + +**Goal:** Change `vertex_descriptor::vertex_id()`, `edge_descriptor::source_id()`, and +`edge_descriptor::target_id()` from `auto` (by value) to `decltype(auto)` so that map-based +graphs return a `const&` to the stable key rather than copying it. This is the change that +makes `raw_vertex_id_t` return a reference for map-based graphs. + +**Risk level: MEDIUM.** Returning a reference from a descriptor method can cause dangling +references if the descriptor is a temporary. Every call site must be audited. + +### Step 1.1: Audit temporary descriptor usage + +**Action (read-only — no code changes):** Search for all call sites where `vertex_id()`, +`source_id()`, or `target_id()` are called on a temporary descriptor. Patterns to search: + +``` +vertex_id() — called on vertex_descriptor returned by *find_vertex(g, uid) +source_id() — called on edge_descriptor values +target_id() — called on edge_descriptor values +``` + +Verify that in every case, the descriptor is either: +- Bound to a named variable (safe — reference extends lifetime), or +- The result is immediately copied into a `vertex_id_t` local (safe — copy), or +- Used in an expression where the descriptor's lifetime spans the use (safe). + +Document any unsafe patterns found. They must be fixed before proceeding. + +**Validation:** No build needed — this is a review step. + +### Step 1.2: Change `vertex_descriptor::vertex_id()` to `decltype(auto)` + +**File:** `include/graph/adj_list/vertex_descriptor.hpp` + +**Change:** `auto` → `decltype(auto)`. Parenthesize the keyed-storage return: +```cpp +[[nodiscard]] constexpr decltype(auto) vertex_id() const noexcept { + if constexpr (std::random_access_iterator) { + return storage_; // prvalue (integral), unchanged + } else { + return (std::get<0>(*storage_)); // lvalue ref to map key + } +} +``` + +**Validation:** +- Build: `linux-gcc-debug`, `linux-clang-debug`, `linux-gcc-asan` +- Test targeted: container tests (`tests/container/dynamic_graph/`), especially + `test_dynamic_graph_integration`, `test_dynamic_graph_nonintegral_ids`, + `test_dynamic_graph_cpo_map_vertices`, `test_dynamic_graph_cpo_unordered_map_vertices` +- Test: `ctest --test-dir build/linux-gcc-asan --output-on-failure` (full, to catch + dangling references via ASan) + +### Step 1.3: Change `edge_descriptor::source_id()` and `target_id()` to `decltype(auto)` + +**File:** `include/graph/adj_list/edge_descriptor.hpp` + +**Change:** Same pattern as Step 1.2. `source_id()` delegates to `vertex_descriptor::vertex_id()` +so it may only need the return type change. `target_id()` has multiple extraction branches — +parenthesize only the keyed-storage branches. + +**Validation:** Same as Step 1.2. + +### Step 1.4: Verify `raw_vertex_id_t` now resolves to a reference for map-based graphs + +**Action:** Add a temporary `static_assert` in one test file (e.g., +`test_dynamic_graph_integration.cpp`) to confirm: +```cpp +// For vector-based graph: +static_assert(!std::is_reference_v>); +// For map-based graph: +static_assert(std::is_reference_v>); +``` + +**Validation:** Build + test. Remove the temporary `static_assert` after confirming (or keep +it as a permanent regression guard). + +### Step 1.5: Phase 1 gate — full matrix build with ASan and commit + +**Commands:** +```bash +./scripts/test-all.sh +cmake --build build/linux-gcc-asan -j $(nproc) && ctest --test-dir build/linux-gcc-asan --output-on-failure +git add -A && git commit -m "Phase 1: Descriptor return types to decltype(auto) for reference semantics" +``` + +**Exit criterion:** All presets green. ASan reports no errors. + +--- + +## Phase 2: Concept and Trait Definitions + +**Goal:** Change `vertex_id_t uid` → `const vertex_id_t& uid` in requires-expressions +of concepts and trait-detection concepts. This ensures concepts match the `const&` calling +convention that the rest of the library will adopt. + +**Risk level: LOW-MEDIUM.** Concept changes can affect overload resolution. The key safety +measure is to verify concept satisfaction for all existing graph container types after each step. + +### Step 2.1: Update `adjacency_list_traits.hpp` + +**File:** `include/graph/adj_list/adjacency_list_traits.hpp` + +**Change:** In every trait-detection concept that takes `vertex_id_t uid`, change to +`const vertex_id_t& uid`: +- `has_degree_uid_impl` +- `has_find_vertex_impl` +- `has_find_vertex_edge_uidvid_impl` +- `has_contains_edge` +- Any others found in the file + +**Validation:** +- Build: `linux-gcc-debug`, `linux-clang-debug` +- Test: `ctest --test-dir build/linux-gcc-debug --output-on-failure` (full — concept changes + can have surprising effects) + +### Step 2.2: Update `adjacency_list_concepts.hpp` + +**File:** `include/graph/adj_list/adjacency_list_concepts.hpp` + +**Change:** In every concept that takes `vertex_id_t uid` in a requires-expression, change +to `const vertex_id_t& uid`: +- `vertex` +- `index_vertex_range` +- `adjacency_list` +- `bidirectional_adjacency_list` +- Any others found in the file + +**Validation:** +- Build: `linux-gcc-debug`, `linux-clang-debug` +- Test: full test suite. Pay special attention to `tests/adj_list/` and `tests/container/` — + these exercise concept satisfaction directly. + +### Step 2.3: Phase 2 gate — full matrix build and commit + +**Commands:** +```bash +./scripts/test-all.sh +git add -A && git commit -m "Phase 2: Concept and trait requires-expressions use const vertex_id_t&" +``` + +**Exit criterion:** All presets green. + +--- + +## Phase 3: View Factory Functions and Constructors + +**Goal:** Change all view factory functions and view class constructors from +`vertex_id_t uid` to `const vertex_id_t& uid`. This is a high-count but low-risk +mechanical change. + +**Approach:** Process one view family at a time, build+test after each. This limits blast +radius if a change introduces an issue. + +### Step 3.1: `incidence.hpp` — factory functions and constructors + +**File:** `include/graph/views/incidence.hpp` + +**Change:** All factory function overloads (`incidence`, `basic_incidence`, `out_incidence`, +`basic_out_incidence`, `in_incidence`, `basic_in_incidence`) and view class constructors: +`vertex_id_t uid` → `const vertex_id_t& uid`. + +**Validation:** +- Build: `linux-gcc-debug` +- Test: `tests/views/test_incidence.cpp`, `tests/views/test_basic_incidence.cpp`, + `tests/views/test_in_incidence.cpp` + +### Step 3.2: `neighbors.hpp` — factory functions and constructors + +**File:** `include/graph/views/neighbors.hpp` + +**Change:** Same pattern — all `neighbors`, `basic_neighbors`, `out_neighbors`, +`basic_out_neighbors`, `in_neighbors`, `basic_in_neighbors` overloads. + +**Validation:** +- Build: `linux-gcc-debug` +- Test: `tests/views/test_neighbors.cpp`, `tests/views/test_basic_neighbors.cpp`, + `tests/views/test_in_neighbors.cpp` + +### Step 3.3: `bfs.hpp` — factory functions and constructors + +**File:** `include/graph/views/bfs.hpp` + +**Change:** All `vertices_bfs` and `edges_bfs` factory overloads, plus `vertices_bfs_view` +and `edges_bfs_view` constructors. + +**Validation:** +- Build: `linux-gcc-debug` +- Test: `tests/views/test_bfs.cpp` + +### Step 3.4: `dfs.hpp` — factory functions and constructors + +**File:** `include/graph/views/dfs.hpp` + +**Change:** All `vertices_dfs` and `edges_dfs` factory overloads, plus view constructors. + +**Validation:** +- Build: `linux-gcc-debug` +- Test: `tests/views/test_dfs.cpp` + +### Step 3.5: `transpose.hpp` — friend functions + +**File:** `include/graph/views/transpose.hpp` + +**Change:** `find_vertex(transpose_view&, vertex_id_t uid)` → +`find_vertex(transpose_view&, const vertex_id_t& uid)` (both const and non-const overloads). + +**Validation:** +- Build: `linux-gcc-debug` +- Test: `tests/views/test_transpose.cpp` + +### Step 3.6: `topological_sort.hpp` (view) — if it takes vertex_id + +**File:** `include/graph/views/topological_sort.hpp` + +**Change:** Check if this view takes `vertex_id_t` parameters. If so, apply same change. + +**Validation:** Build + test `tests/views/test_topological_sort.cpp`. + +### Step 3.7: `vertexlist.hpp` and `edgelist.hpp` — check for vertex_id parameters + +**Files:** `include/graph/views/vertexlist.hpp`, `include/graph/views/edgelist.hpp` + +**Change:** Check if any factory functions take `vertex_id_t` parameters (e.g., subrange +overloads with uid bounds). If so, apply same change. + +**Validation:** Build + test relevant view tests. + +### Step 3.8: Phase 3 gate — full matrix build, benchmarks, and commit + +**Commands:** +```bash +./scripts/test-all.sh +cmake --build build/linux-gcc-release -j $(nproc) +./build/linux-gcc-release/benchmark/graph3_benchmarks +git add -A && git commit -m "Phase 3: View factory functions and constructors use const vertex_id_t&" +``` + +**Exit criterion:** All presets green. Benchmarks show no regression. + +--- + +## Phase 4a: Algorithm Parameter Signatures + +**Goal:** Change algorithm function parameters from `vertex_id_t source` to +`const vertex_id_t& source`, and internal lambda parameters similarly. This phase does +**not** change internal storage — that is Phase 4b. + +**Approach:** One algorithm at a time. Each algorithm has its own test file, so validation is +straightforward. + +### Step 4a.1: `dijkstra_shortest_paths.hpp` + +**File:** `include/graph/algorithm/dijkstra_shortest_paths.hpp` + +**Changes:** +- Multi-source: no `vertex_id_t` parameter (takes `Sources` range) — no change to signature. +- Single-source `dijkstra_shortest_paths`: `vertex_id_t source` → `const vertex_id_t& source` +- Single-source `dijkstra_shortest_distances`: same change +- `relax_target` lambda: `vertex_id_t uid` → `const vertex_id_t& uid` + +**Validation:** +- Build: `linux-gcc-debug` +- Test: `tests/algorithms/test_dijkstra_shortest_paths.cpp` + +### Step 4a.2: `bellman_ford_shortest_paths.hpp` + +**File:** `include/graph/algorithm/bellman_ford_shortest_paths.hpp` + +**Changes:** Same pattern — single-source overloads and `relax_target` lambda. + +**Validation:** Build + test `tests/algorithms/test_bellman_ford_shortest_paths.cpp`. + +### Step 4a.3: `breadth_first_search.hpp` + +**File:** `include/graph/algorithm/breadth_first_search.hpp` + +**Changes:** Single-source overload: `vertex_id_t source` → `const vertex_id_t& source`. + +**Validation:** Build + test `tests/algorithms/test_breadth_first_search.cpp`. + +### Step 4a.4: `depth_first_search.hpp` — verify, no change expected + +**File:** `include/graph/algorithm/depth_first_search.hpp` + +**Action:** Verify it already uses `const vertex_id_t& source`. No change expected. + +**Validation:** Build + test `tests/algorithms/test_depth_first_search.cpp`. + +### Step 4a.5: `topological_sort.hpp` + +**File:** `include/graph/algorithm/topological_sort.hpp` + +**Changes:** Single-source overload: `vertex_id_t source` → `const vertex_id_t& source`. + +**Validation:** Build + test `tests/algorithms/test_topological_sort.cpp`. + +### Step 4a.6: `mis.hpp` + +**File:** `include/graph/algorithm/mis.hpp` + +**Changes:** `vertex_id_t seed = 0` — evaluate whether to change to +`const vertex_id_t& seed`. Since this algorithm requires `index_adjacency_list` (integral +IDs), the default `= 0` is valid. If changing to `const&`, the default becomes +`= vertex_id_t{}`. Choose the option that compiles cleanly. + +**Validation:** Build + test `tests/algorithms/test_mis.cpp`. + +### Step 4a.7: `connected_components.hpp` + +**File:** `include/graph/algorithm/connected_components.hpp` + +**Changes:** Check all function signatures for `vertex_id_t` parameters. Internal lambdas +taking vertex_id by value should change to `const vertex_id_t&`. + +**Validation:** Build + test `tests/algorithms/test_connected_components.cpp`. + +### Step 4a.8: `label_propagation.hpp` + +**File:** `include/graph/algorithm/label_propagation.hpp` + +**Changes:** Check for `vertex_id_t` parameters. Apply same pattern. + +**Validation:** Build + test `tests/algorithms/test_label_propagation.cpp`. + +### Step 4a.9: Remaining algorithms — `mst.hpp`, `jaccard.hpp`, `tc.hpp`, `articulation_points.hpp`, `biconnected_components.hpp` + +**Files:** All remaining algorithm headers. + +**Changes:** Check each for `vertex_id_t` function/lambda parameters. Change to `const&` +where applicable. Loop variables (`for (vertex_id_t uid = 0; ...)`) remain by value. + +**Validation:** Build + test each algorithm's test file. + +### Step 4a.10: Phase 4a gate — full matrix build and commit + +**Commands:** +```bash +./scripts/test-all.sh +git add -A && git commit -m "Phase 4a: Algorithm parameter signatures use const vertex_id_t&" +``` + +**Exit criterion:** All presets green. + +--- + +## Phase 4b: Algorithm Internal Storage (`vertex_id_store_t`) + +**Goal:** Change algorithm-internal `using id_type = vertex_id_t` to +`using id_type = vertex_id_store_t`. Add `static_assert` guards. + +**Risk note:** For all current algorithms constrained to `index_adjacency_list`, this is a +no-op — `vertex_id_store_t` resolves to `vertex_id_t` for integral IDs. The +`static_assert` guards verify this. This phase establishes the pattern for future algorithms +that may relax the integral constraint. + +### Step 4b.1: `dijkstra_shortest_paths.hpp` + +**File:** `include/graph/algorithm/dijkstra_shortest_paths.hpp` + +**Change:** +```cpp +// Before: +using id_type = vertex_id_t; + +// After: +using id_type = vertex_id_store_t; +// Guard: for index_adjacency_list, id_type must be the value type +static_assert(std::is_same_v>, + "vertex_id_store_t should equal vertex_id_t for index_adjacency_list"); +``` + +**Validation:** +- Build: `linux-gcc-debug`, `linux-clang-debug` +- Test: `tests/algorithms/test_dijkstra_shortest_paths.cpp` + +### Step 4b.2: `bellman_ford_shortest_paths.hpp` + +**File + Change:** Same pattern. + +**Validation:** Build + test. + +### Step 4b.3: `breadth_first_search.hpp` + +**File + Change:** Same pattern (if it has `using id_type = vertex_id_t`). + +**Validation:** Build + test. + +### Step 4b.4: `depth_first_search.hpp` + +**File + Change:** Same pattern. + +**Validation:** Build + test. + +### Step 4b.5: Remaining algorithms + +**Files:** All remaining algorithm headers with `using id_type = vertex_id_t` or equivalent. + +**Change:** Same pattern with `static_assert` guard. + +**Validation:** Build + test each. + +### Step 4b.6: Phase 4b gate — full matrix build with ASan and commit + +**Commands:** +```bash +./scripts/test-all.sh +cmake --build build/linux-gcc-asan -j $(nproc) && ctest --test-dir build/linux-gcc-asan --output-on-failure +git add -A && git commit -m "Phase 4b: Algorithm internals use vertex_id_store_t with static_assert guards" +``` + +**Exit criterion:** All presets green. ASan clean. + +--- + +## Phase 5: Adaptor / Pipe Syntax + +**Goal:** Review and update adaptor functions in `adaptors.hpp` to ensure forwarding works +correctly with `const vertex_id_t&` arguments. + +### Step 5.1: Review and update `adaptors.hpp` + +**File:** `include/graph/views/adaptors.hpp` + +**Action:** Review each adaptor that forwards a vertex ID. These typically use: +```cpp +adj_list::vertex_id_t>(std::forward(uid)) +``` +Verify that `UID` correctly deduces and forwards when the caller passes `const vertex_id_t&`. +If any adaptor explicitly constructs a `vertex_id_t` value, that is intentional (copying +for storage) and should be left as-is. + +**Validation:** +- Build: `linux-gcc-debug` +- Test: `tests/views/test_adaptors.cpp` + +### Step 5.2: Phase 5 gate — full matrix build and commit + +**Commands:** +```bash +./scripts/test-all.sh +git add -A && git commit -m "Phase 5: Adaptor/pipe syntax functions updated for const& vertex IDs" +``` + +**Exit criterion:** All presets green. + +--- + +## Phase 6: Documentation and Examples + +**Goal:** Update documentation and examples to reflect the new `const&` convention. + +### Step 6.1: Update examples + +**Files:** `examples/dijkstra_clrs_example.cpp`, `examples/basic_usage.cpp`, +`examples/mst_usage_example.cpp` + +**Change:** If any example shows a function signature with `vertex_id_t` by value, update +to `const vertex_id_t&`. Update any prose comments. + +**Validation:** Build examples — `cmake --build build/linux-gcc-debug --target examples`. + +### Step 6.2: Update user-guide documentation + +**Files:** `docs/user-guide/*.md`, `docs/reference/*.md` + +**Change:** Search for `vertex_id_t` in documentation and update any API signature descriptions. +Add a note about the `const&` convention and `vertex_id_store_t` for algorithm implementers. + +**Validation:** Manual review (documentation only). + +### Step 6.3: Phase 6 gate — full matrix build and commit + +**Commands:** +```bash +./scripts/test-all.sh +git add -A && git commit -m "Phase 6: Documentation and examples updated for const& vertex ID convention" +``` + +**Exit criterion:** All presets green. All documentation accurate. + +--- + +## Final Validation + +After all phases are complete: + +1. **Full matrix build:** `./scripts/test-all.sh` +2. **ASan:** `cmake --build build/linux-gcc-asan -j $(nproc) && ctest --test-dir build/linux-gcc-asan --output-on-failure` +3. **Benchmarks:** Compare before/after on `linux-gcc-release` to verify no performance regression +4. **Map-based graph tests:** Specifically run `test_dynamic_graph_integration`, + `test_dynamic_graph_nonintegral_ids`, and `test_dynamic_graph_cpo_map_vertices` under ASan +5. **Merge:** `git checkout main && git merge --no-ff feature/vertex-id-const-ref` + +--- + +## Status Summary + +| Phase | Step | Description | Status | +|-------|------|-------------|--------| +| **0** | 0.1 | Protect `vertex_id_t` in `graph_cpo.hpp`, add `raw_vertex_id_t` | **completed** | +| **0** | 0.2 | Protect `vertex_id_t` in `edge_list.hpp` | **completed** | +| **0** | 0.3 | Add `vertex_id_store_t` in `traversal_common.hpp` | **completed** | +| **0** | 0.4 | Phase 0 gate — full matrix build | **completed** | +| **1** | 1.1 | Audit temporary descriptor usage (read-only) | **completed** | +| **1** | 1.2 | `vertex_descriptor::vertex_id()` → `decltype(auto)` | **completed** | +| **1** | 1.3 | `edge_descriptor::source_id()` and `target_id()` → `decltype(auto)` | **completed** | +| **1** | 1.4 | Verify `raw_vertex_id_t` resolves correctly (static_assert) | **completed** | +| **1** | 1.5 | Phase 1 gate — full matrix + ASan | **completed** | +| **2** | 2.1 | Update `adjacency_list_traits.hpp` concepts to `const&` | **completed** | +| **2** | 2.2 | Update `adjacency_list_concepts.hpp` concepts to `const&` | **completed** | +| **2** | 2.3 | Phase 2 gate — full matrix build | **completed** | +| **3** | 3.1 | `incidence.hpp` — factory functions and constructors | **completed** | +| **3** | 3.2 | `neighbors.hpp` — factory functions and constructors | **completed** | +| **3** | 3.3 | `bfs.hpp` — factory functions and constructors | **completed** | +| **3** | 3.4 | `dfs.hpp` — factory functions and constructors | **completed** | +| **3** | 3.5 | `transpose.hpp` — friend functions | **completed** | +| **3** | 3.6 | `topological_sort.hpp` (view) — check and update | **completed** (no changes needed) | +| **3** | 3.7 | `vertexlist.hpp` and `edgelist.hpp` — check and update | **completed** (no changes needed) | +| **3** | 3.8 | Phase 3 gate — full matrix + benchmarks | **completed** | +| **4a** | 4a.1 | `dijkstra_shortest_paths.hpp` — signatures + lambdas | **completed** | +| **4a** | 4a.2 | `bellman_ford_shortest_paths.hpp` — signatures + lambdas | **completed** | +| **4a** | 4a.3 | `breadth_first_search.hpp` — signature | **completed** | +| **4a** | 4a.4 | `depth_first_search.hpp` — verify (no change expected) | **completed** | +| **4a** | 4a.5 | `topological_sort.hpp` — signature | **completed** | +| **4a** | 4a.6 | `mis.hpp` — signature (evaluate default arg) | **completed** | +| **4a** | 4a.7 | `connected_components.hpp` — signatures + lambdas | **completed** | +| **4a** | 4a.8 | `label_propagation.hpp` — signatures | **completed** | +| **4a** | 4a.9 | Remaining algorithms (`mst`, `jaccard`, `tc`, `articulation_points`, `biconnected_components`) | **completed** | +| **4a** | 4a.10 | Phase 4a gate — full matrix build | **completed** | +| **4b** | 4b.1 | `dijkstra_shortest_paths.hpp` — `id_type` + static_assert | **completed** | +| **4b** | 4b.2 | `bellman_ford_shortest_paths.hpp` — `id_type` + static_assert | **completed** | +| **4b** | 4b.3 | `breadth_first_search.hpp` — `id_type` + static_assert | **completed** | +| **4b** | 4b.4 | `depth_first_search.hpp` — `id_type` + static_assert | **completed** | +| **4b** | 4b.5 | Remaining algorithms — `id_type` + static_assert | **completed** | +| **4b** | 4b.6 | Phase 4b gate — full matrix + ASan | **completed** | +| **5** | 5.1 | Review and update `adaptors.hpp` | **completed** (no changes needed) | +| **5** | 5.2 | Phase 5 gate — full matrix build | **completed** | +| **6** | 6.1 | Update examples | **completed** | +| **6** | 6.2 | Update user-guide documentation | **completed** | +| **6** | 6.3 | Phase 6 gate — full matrix build | **completed** | +| — | — | **Final validation** — full matrix + ASan + benchmarks | not-started | diff --git a/agents/vertex_id_const_ref_strategy.md b/agents/vertex_id_const_ref_strategy.md new file mode 100644 index 0000000..01c48b1 --- /dev/null +++ b/agents/vertex_id_const_ref_strategy.md @@ -0,0 +1,707 @@ +# Strategy: Pass `vertex_id_t` by `const&` Throughout the Library + +## Problem Statement + +`vertex_id_t` is defined as the return type of the `vertex_id(g, u)` CPO. While it is +typically an integral value (e.g. `size_t`) for vector-based graphs, it can be **any type** — for +example, `std::string` is already used as a vertex ID in map-based graph containers +(`mos_graph_traits`, `mol_graph_traits`, etc.) and exercised in integration tests. + +Currently, `vertex_id_t` is passed **by value** in the vast majority of the codebase: +algorithms, view factory functions, concept `requires`-expressions, and internal lambda captures. +This is correct for integral types but introduces **unnecessary copies** for non-trivial types like +`std::string`. The library should consistently pass vertex IDs by `const vertex_id_t&` where +the value is only read, and use forwarding references (`auto&&` / `VId&&`) where ownership transfer +or move semantics are beneficial. + +## Current State Audit + +### Where `vertex_id_t` Is **Already** Passed by `const&` + +| Location | Convention | Notes | +|----------|-----------|-------| +| **CPOs** (`find_vertex`, `edges`, `degree`, `in_edges`, `in_degree`) | `const VId& uid` | All CPOs in `graph_cpo.hpp` already use `const VId&` for vertex ID parameters | +| **Visitor concepts** (`traversal_common.hpp`) | `const vertex_id_t& uid` | `has_on_discover_vertex_id`, `has_on_examine_vertex_id`, etc. | +| **`depth_first_search` algorithm** | `const vertex_id_t& source` | The only algorithm that does this | +| **Container member functions** (`dynamic_graph`) | `const vertex_id_type& id` | `find_vertex()`, `try_find_vertex()` | + +### Where `vertex_id_t` Is Passed **by Value** (Needs Change) + +| Category | Count | Example | +|----------|-------|---------| +| **View factory functions** (incidence, neighbors, bfs, dfs, etc.) | ~40+ overloads | `incidence(G& g, vertex_id_t uid)` | +| **Algorithms** (all except DFS) | ~15 signatures | `dijkstra_shortest_paths(G&&, vertex_id_t source, ...)` | +| **Concept requires-expressions** | ~10 concepts | `vertex` concept uses `vertex_id_t uid` | +| **Trait detection concepts** | ~8 concepts | `has_degree_uid_impl`, `has_find_vertex_impl` | +| **View class constructors** | ~10 | `vertices_bfs_view(G& g, vertex_id_type seed, ...)` | +| **Internal local variables** | many | `vertex_id_t uid = vertex_id(g, u);` — these are fine by-value | +| **Transpose view friend functions** | 2 | `find_vertex(transpose_view&, vertex_id_t uid)` | + +### Where `vertex_id_t` Is **Returned** by Value + +| Location | Returns | Notes | +|----------|---------|-------| +| `vertex_descriptor::vertex_id()` | `auto` (by value) | Has existing TODO comment to change to `decltype(auto)` | +| `edge_descriptor::source_id()` | `auto` (by value) | Delegates to `vertex_descriptor::vertex_id()` | +| `edge_descriptor::target_id()` | `auto` (by value) | All extraction branches return copies | +| CPO `vertex_id(g, u)` | `decltype(auto)` | Currently resolves to value because underlying methods return `auto` | +| CPO `target_id(g, uv)` | `decltype(auto)` | Currently resolves to value for same reason | +| CPO `source_id(g, uv)` | value | Same issue | + +## Requirements + +### R1: Parameters — Use `const vertex_id_t&` for Read-Only Vertex IDs + +Any function parameter of type `vertex_id_t` that is only read (not modified, not necessarily +stored) should become `const vertex_id_t&`. This avoids copies for non-trivial ID types while +being identical in performance for integral types (compilers elide the indirection for small types +passed by const-ref). + +**Scope:** Algorithm signatures, view factory functions, view class constructors, concept +requires-expressions, trait detection concepts, transpose view friend functions. + +### R2: Storage — Use `vertex_id_store_t` for Efficient Internal Storage + +Local variables declared for temporary use and container elements that must outlive the graph +(e.g., returned results) should continue to store `vertex_id_t` **by value**. + +However, algorithm-internal containers (queues, stacks, priority queues, visited-set keys) that +exist only during a traversal can exploit a key invariant: **the graph must be stable while the +algorithm runs.** For map-based graphs, vertex ID keys are embedded in stable map nodes whose +addresses do not change during iteration. A `std::reference_wrapper` pointing at such a key is +valid for the entire algorithm lifetime and is trivially copyable (pointer-sized, 8 bytes), +eliminating per-enqueue copies of non-trivial types like `std::string`. + +The pattern uses a compile-time conditional: + +```cpp +// Defined in a common algorithm header (see R6). +template +using vertex_id_store_t = std::conditional_t< + std::is_reference_v>, + std::reference_wrapper>>, + vertex_id_t>; +``` + +For integral IDs (`size_t`), `raw_vertex_id_t` is a prvalue so `is_reference_v` is false, +and `vertex_id_store_t` is just `size_t` — zero overhead, identical codegen. + +For map-based IDs (`const std::string&`), `is_reference_v` is true, and +`vertex_id_store_t` is `std::reference_wrapper` — 8 bytes, trivially +copyable, implicitly convertible back to `const std::string&`. + +Algorithm-internal data structures use this type: +```cpp +using id_type = vertex_id_store_t; +std::priority_queue, Cmp> queue; +std::queue frontier; +``` + +Assignments from `const vertex_id_t&` parameters to `vertex_id_store_t` local variables +work naturally: for integral types it copies the value; for reference types `reference_wrapper` +captures the reference. + +### R3: Return Types — Use `decltype(auto)` with Parenthesized Returns + +`vertex_descriptor::vertex_id()`, `edge_descriptor::source_id()`, and +`edge_descriptor::target_id()` should return `decltype(auto)` instead of `auto`. For map-based +storage where the key is embedded in the iterator's value, this enables **returning a const +reference** to the key rather than copying it. + +The implementation uses parenthesized return expressions: +```cpp +// Before: +[[nodiscard]] constexpr auto vertex_id() const noexcept { + return std::get<0>(*storage_); // copies the key +} + +// After: +[[nodiscard]] constexpr decltype(auto) vertex_id() const noexcept { + return (std::get<0>(*storage_)); // returns const& to the key +} +``` + +For random-access (integral) cases, `return storage_;` already returns an integral prvalue — no +parentheses needed there, and adding them would only bind to a `const size_t&` to a local, which +is fine with `decltype(auto)` since the caller typically copies into a local `vertex_id_t` +anyway. **However**, returning a reference to a local/member is dangerous if the descriptor is a +temporary. This needs careful analysis per code path. A safer approach: only parenthesize the +keyed-storage branch using `if constexpr`. + +### R4: Forwarding References in Algorithms (Selective) + +In algorithms that accept sources as a range, or where a vertex ID may be constructed in-place, +a forwarding reference `auto&& uid` (or `VId&& uid` with `std::forward`) may be appropriate. This +is a **secondary optimization** — the primary change should be `const&`. + +Candidate sites: +- Multi-source algorithm overloads where a `std::initializer_list` or range of IDs is passed +- Adaptor pipe-syntax functions that forward to view constructors + +### R5: `vertex_id_t` Type Alias Must Remain a Value Type + +The type alias `vertex_id_t` itself (in `graph_cpo.hpp` and `edge_list.hpp`) should remain a +**value type** (i.e., the `remove_cvref_t` of the return). It represents the canonical value type +of vertex IDs, not a reference type. Code that needs `const&` semantics applies `const&` at the +use site. + +If R3 changes cause `vertex_id(g, u)` to return a reference, the `vertex_id_t` alias +definition should apply `std::remove_cvref_t`: + +```cpp +template +using vertex_id_t = std::remove_cvref_t< + decltype(vertex_id(std::declval(), std::declval>()))>; +``` + +This may already be the case for the edge_list definition but must be made consistent. + +### R6: Introduce `raw_vertex_id_t` and `vertex_id_store_t` Aliases + +Two new type aliases are needed alongside `vertex_id_t`: + +```cpp +// Preserves reference-ness from the CPO return. For vector-based graphs this is +// size_t (prvalue). For map-based graphs this is const std::string& (lvalue ref). +template +using raw_vertex_id_t = decltype(vertex_id(std::declval(), std::declval>())); + +// Efficient storage type for algorithm internals. For integral IDs this is the +// value type itself. For reference-returning IDs this is a reference_wrapper, +// which is trivially copyable (pointer-sized) and implicitly converts back to +// const ref. +template +using vertex_id_store_t = std::conditional_t< + std::is_reference_v>, + std::reference_wrapper>>, + vertex_id_t>; +``` + +The three aliases form a consistent family: + +| Alias | Type for vector graph | Type for map graph | Use case | +|-------|----------------------|-------------------|----------| +| `vertex_id_t` | `size_t` | `std::string` | Value type — parameters (`const&`), result containers, type constraints | +| `raw_vertex_id_t` | `size_t` | `const std::string&` | Raw CPO return — used only in `conditional_t` and SFINAE | +| `vertex_id_store_t` | `size_t` | `reference_wrapper` | Algorithm-internal queues, stacks, priority queues | + +## Implementation Plan + +### Phase 0: Type Aliases — Protect `vertex_id_t`, Add `raw_vertex_id_t` and `vertex_id_store_t` + +**Files:** `include/graph/adj_list/detail/graph_cpo.hpp`, `include/graph/edge_list/edge_list.hpp`, +and a common algorithm header (e.g., `include/graph/algorithm/common.hpp` or `traversal_common.hpp`). + +1. Wrap both `vertex_id_t` definitions in `std::remove_cvref_t<>` to ensure they remain value + types regardless of whether underlying returns change to `decltype(auto)`. + +2. Add `raw_vertex_id_t` alongside `vertex_id_t` in the same headers. This alias preserves + the reference-ness of the CPO return: + ```cpp + template + using raw_vertex_id_t = decltype(vertex_id(std::declval(), std::declval>())); + ``` + +3. Add `vertex_id_store_t` in a header visible to all algorithms: + ```cpp + template + using vertex_id_store_t = std::conditional_t< + std::is_reference_v>, + std::reference_wrapper>>, + vertex_id_t>; + ``` + +**Risk:** Low. `remove_cvref_t` on a value type is a no-op. The new aliases are additive. + +**Validation:** Build all presets. Run full test suite. + +--- + +### Phase 1: Descriptor Return Types (`decltype(auto)`) + +**Files:** +- `include/graph/adj_list/vertex_descriptor.hpp` — `vertex_id()` +- `include/graph/adj_list/edge_descriptor.hpp` — `source_id()`, `target_id()` + +**Changes:** +1. Change `auto` → `decltype(auto)` on the three methods. +2. In the keyed-storage branch (map iterators), parenthesize + return expressions to enable reference binding: + ```cpp + if constexpr (std::random_access_iterator) { + return storage_; // prvalue (integral) — no change + } else { + return (std::get<0>(*storage_)); // lvalue ref to key + } + ``` +3. Verify that all callers handle both prvalue and lvalue returns correctly. The + `vertex_id_t` alias (Phase 0) strips references, so code using the alias as a local + variable type will still copy — which is correct for storage. + +**Risk:** Medium. `decltype(auto)` propagating a reference to a temporary is dangerous. Must audit +all sites where `vertex_id()` is called on a temporary descriptor. If any exist, the descriptor +must be named (bound to a variable) first. + +**Validation:** Build all presets (especially with `-fsanitize=address`). Run full test suite +including `test_dynamic_graph_integration.cpp` which tests string vertex IDs. + +--- + +### Phase 2: Concept and Trait Definitions + +**Files:** +- `include/graph/adj_list/adjacency_list_concepts.hpp` +- `include/graph/adj_list/adjacency_list_traits.hpp` + +**Changes in `adjacency_list_concepts.hpp`:** + +Change requires-expression variables from `vertex_id_t uid` to +`const vertex_id_t& uid` in all concepts that only read the ID: + +```cpp +// Before (vertex concept, ~L103): +concept vertex = requires(G&& g, vertex_t u, vertex_id_t uid) { ... }; + +// After: +concept vertex = requires(G&& g, vertex_t u, const vertex_id_t& uid) { ... }; +``` + +Similarly for: `index_vertex_range`, `adjacency_list`, `bidirectional_adjacency_list`, etc. + +**Changes in `adjacency_list_traits.hpp`:** + +Change all trait detection concepts: +```cpp +// Before (~L32): +concept has_degree_uid_impl = requires(G&& g, vertex_id_t uid) { ... }; + +// After: +concept has_degree_uid_impl = requires(G&& g, const vertex_id_t& uid) { ... }; +``` + +Similarly for: `has_find_vertex_impl`, `has_find_vertex_edge_uidvid_impl`, +`has_contains_edge`, etc. + +**Risk:** Low-Medium. Changing concept constraints may cause overload resolution changes if +container member functions expect `vertex_id_type` by value rather than `const&`. Must verify +that all concrete container member functions that participate in concept satisfaction accept +`const vertex_id_type&` (they already do for `dynamic_graph`). + +**Validation:** Build all presets. Run full test suite. Specifically verify concept satisfaction for +all graph container types (vector-based and map-based). + +--- + +### Phase 3: View Factory Functions and View Constructors + +**Files:** +- `include/graph/views/incidence.hpp` — ~14 factory overloads + 2 constructors +- `include/graph/views/neighbors.hpp` — ~14 factory overloads + 2 constructors +- `include/graph/views/bfs.hpp` — ~12 factory overloads + 4 constructors +- `include/graph/views/dfs.hpp` — ~12 factory overloads + 4 constructors +- `include/graph/views/transpose.hpp` — 2 friend functions + +**Pattern for factory functions:** +```cpp +// Before: +template +[[nodiscard]] constexpr auto incidence(G& g, adj_list::vertex_id_t uid) { ... } + +// After: +template +[[nodiscard]] constexpr auto incidence(G& g, const adj_list::vertex_id_t& uid) { ... } +``` + +**Pattern for view constructors:** +```cpp +// Before: +vertices_bfs_view(G& g, vertex_id_type seed, Alloc alloc = {}) + +// After: +vertices_bfs_view(G& g, const vertex_id_type& seed, Alloc alloc = {}) +``` + +Note: The constructor body stores `seed` into a member or pushes it into a queue — that copy is +intentional and correct (R2). Only the parameter binding changes. + +**Risk:** Low. These are direct signature changes. Internal usage copies the ID into queues/stacks +which remains by-value. + +**Validation:** Build all presets. Run full test suite, view tests, and benchmark suite. + +--- + +### Phase 4: Algorithm Signatures and Internal Storage + +**Files:** +- `include/graph/algorithm/dijkstra_shortest_paths.hpp` +- `include/graph/algorithm/bellman_ford_shortest_paths.hpp` +- `include/graph/algorithm/breadth_first_search.hpp` +- `include/graph/algorithm/topological_sort.hpp` +- `include/graph/algorithm/mis.hpp` +- `include/graph/algorithm/connected_components.hpp` +- `include/graph/algorithm/label_propagation.hpp` + +This phase has two sub-parts: (a) parameter signatures, and (b) internal storage via +`vertex_id_store_t`. + +#### Phase 4a: Parameter Signatures — `const vertex_id_t&` + +**Pattern for single-source algorithms:** +```cpp +// Before: +template +void dijkstra_shortest_paths(G&& g, vertex_id_t source, ...) { ... } + +// After: +template +void dijkstra_shortest_paths(G&& g, const vertex_id_t& source, ...) { ... } +``` + +**Internal lambdas** (e.g. `relax_target` in Dijkstra/Bellman-Ford): +```cpp +// Before: +auto relax_target = [&](const edge_t& e, vertex_id_t uid, const weight_type& w_e) -> bool { + +// After: +auto relax_target = [&](const edge_t& e, const vertex_id_t& uid, const weight_type& w_e) -> bool { +``` + +**Note on `breadth_first_search` single-source wrapper:** +```cpp +// Before: +void breadth_first_search(G&& g, vertex_id_t source, Visitor&& visitor) { + std::array, 1> sources{source}; // copies into array — correct + ... +} + +// After: +void breadth_first_search(G&& g, const vertex_id_t& source, Visitor&& visitor) { + std::array, 1> sources{source}; // still copies — correct + ... +} +``` + +**`depth_first_search`** already uses `const vertex_id_t&` — no change needed. + +**`mis.hpp`** has `vertex_id_t seed = 0` with a default argument. Change to: +```cpp +const vertex_id_t& seed = vertex_id_t{0} +``` +Or, since a default of `0` implies integrality, this function may be exclusively for +`index_adjacency_list` and can be left as-is. Evaluate whether a const-ref default makes sense. + +**Algorithms with loop variables** (`tc.hpp`, `articulation_points.hpp`, +`biconnected_components.hpp`): Loop variables like `for (vertex_id_t uid = 0; ...)` +remain **by value** — they are local storage, not parameters. + +#### Phase 4b: Internal Storage — `vertex_id_store_t` with `reference_wrapper` + +Change algorithm-local `using id_type = vertex_id_t` to +`using id_type = vertex_id_store_t`. + +The graph **must be stable** while an algorithm runs (this is already an implicit precondition +for all traversal algorithms — iterators and edge references are invalidated by mutation). For +map-based graphs, vertex ID keys reside in stable map nodes. A `reference_wrapper` pointing at +such a key is valid for the entire algorithm lifetime. This avoids per-enqueue copies of +non-trivial types while remaining pointer-sized and trivially copyable. + +**Concrete example — `dijkstra_shortest_paths` (multi-source):** + +```cpp +// Before: +constexpr void dijkstra_shortest_paths(G&& g, const Sources& sources, ...) { + using id_type = vertex_id_t; // size_t or string (by value) + using distance_type = range_value_t; + using weight_type = invoke_result_t; + + auto relax_target = [&](const edge_t& e, vertex_id_t uid, ...) -> bool { + const id_type vid = target_id(g, e); // copies string + ... + }; + + const id_type N = static_cast(num_vertices(g)); + + auto qcompare = [&distances](id_type a, id_type b) { // queue elements are strings + return distances[...a] > distances[...b]; + }; + using Queue = std::priority_queue, decltype(qcompare)>; + Queue queue(qcompare); + ... + queue.push(vid); // copies string into queue +} + +// After: +constexpr void dijkstra_shortest_paths(G&& g, const Sources& sources, ...) { + using id_type = vertex_id_store_t; // size_t or reference_wrapper + using distance_type = range_value_t; + using weight_type = invoke_result_t; + + auto relax_target = [&](const edge_t& e, const vertex_id_t& uid, ...) -> bool { + const id_type vid = target_id(g, e); // wraps ref to stable key (8 bytes) + ... + }; + + const id_type N = static_cast(num_vertices(g)); + + auto qcompare = [&distances](id_type a, id_type b) { // queue elements are 8-byte wrappers + return distances[...a] > distances[...b]; // implicit conversion to const string& + }; + using Queue = std::priority_queue, decltype(qcompare)>; + Queue queue(qcompare); + ... + queue.push(vid); // copies 8-byte wrapper, not string +} +``` + +**What changes at each use site inside an algorithm:** + +| Internal use | Before (`vertex_id_t`) | After (`vertex_id_store_t`) | +|---|---|---| +| `id_type vid = target_id(g, e)` | Copies string | Wraps ref (8 bytes) — `target_id` returns `decltype(auto)` ref after Phase 1 | +| `queue.push(vid)` | Copies string into queue | Copies 8-byte wrapper | +| `const id_type uid = queue.top()` | Copies string out | Copies 8-byte wrapper | +| `qcompare(a, b)` | Compares strings by value | `reference_wrapper` implicitly converts to `const string&` | +| `predecessor[vid] = uid` | Copies string | Copies 8-byte wrapper (predecessor stores `id_type` too) | +| `visitor.on_discover_vertex(g, uid)` | Passes string by value | `reference_wrapper` converts to `const vertex_id_t&` matching visitor concepts | +| `distances[static_cast(uid)]` | Direct indexing | Same for integral; N/A for map graphs (separate concern) | + +**Why this is safe:** `reference_wrapper` references the vertex ID key stored inside the graph's +map node. The graph is not mutated during algorithm execution (precondition already documented), +so the map nodes — and their keys — remain at stable addresses. The `reference_wrapper` is valid +from construction until the algorithm returns. + +**Note for `index_adjacency_list`-constrained algorithms (Dijkstra, Bellman-Ford, etc.):** +Since `vertex_id_t` is always integral for these, `is_reference_v>` is +`false`, and `vertex_id_store_t` collapses to `vertex_id_t` — zero overhead, identical +codegen. The `reference_wrapper` path is a zero-cost abstraction that only activates for future +algorithms relaxed to non-integral IDs. + +**Risk:** Low for integral-constrained algorithms (conditional selects the value path — no +behavioral change). Medium for future non-integral algorithms that would be the first to exercise +the `reference_wrapper` path — requires careful testing with map-based graph containers. + +**Validation:** Build all presets. Run full algorithm test suite. For Phase 4b specifically, +add a `static_assert` in each algorithm to verify the expected `id_type` resolution: +```cpp +static_assert(std::is_same_v>); // for index_adjacency_list algorithms +``` +These guards can be relaxed when an algorithm is generalized to non-integral IDs. + +--- + +### Phase 5: Adaptor / Pipe Syntax Functions + +**Files:** +- `include/graph/views/adaptors.hpp` + +These typically use template forwarding: +```cpp +adj_list::vertex_id_t>(std::forward(uid)) +``` + +Review whether `UID` is already a forwarding reference and ensure the forwarding works correctly +with both lvalue (const ref) and rvalue vertex IDs. These may already be correct. + +**Risk:** Low. + +**Validation:** Build and test adaptor/pipe-syntax tests. + +--- + +### Phase 6: Documentation and Examples + +**Files:** +- `docs/user-guide/`, `docs/reference/` +- `examples/dijkstra_clrs_example.cpp`, `examples/basic_usage.cpp` + +Update any API documentation or examples that show `vertex_id_t` by value in function +signatures. + +**Risk:** None (documentation only). + +--- + +## Items Explicitly Out of Scope + +1. **Internal local variables used as loop counters** — `for (vertex_id_t uid = 0; ...)` + remains by value. These are incremented/compared as values. + +2. **Result containers returned to the caller** — `std::vector>` for results + like predecessor arrays or topological orderings must store owned values, since they outlive + the algorithm call and possibly the graph itself. + +3. **Algorithm concept constraints requiring `std::integral`** — Algorithms constrained to + `index_adjacency_list` still require integral vertex IDs. The `const&` and + `vertex_id_store_t` changes are still beneficial for API consistency and future-proofing, + but do not directly unlock non-integral algorithms. Relaxing algorithms to work with + non-integral IDs is a separate effort that builds on this foundation. + +4. **Move semantics for vertex IDs** — Using `vertex_id_t&&` (rvalue reference) for + sink parameters where the caller is done with the ID. The `reference_wrapper` pattern for + internal storage and `const&` for parameters covers the important cases without the API + complexity of rvalue reference overloads. + +## Risk Summary + +| Phase | Risk | Mitigation | +|-------|------|-----------| +| 0 — Type aliases | Low | `remove_cvref_t` is a no-op on value types; new aliases are additive | +| 1 — Descriptor returns | Medium | Audit temporary descriptor usage; sanitizer testing | +| 2 — Concepts/traits | Low-Medium | Verify concept satisfaction for all container types | +| 3 — Views | Low | Straightforward signature changes | +| 4a — Algorithm signatures | Low | Parameter-only changes; bodies unaffected | +| 4b — Algorithm `id_type` | Low (integral) / Medium (future non-integral) | `conditional_t` selects value path for current algorithms; `static_assert` guards | +| 5 — Adaptors | Low | Likely already correct | +| 6 — Documentation | None | — | + +## Build/Test Strategy + +Each phase should: +1. Make the changes +2. Build with **all presets** (`linux-gcc-debug`, `linux-gcc-release`, `linux-clang-debug`, + `linux-clang-release`) +3. Run the **full test suite** (not just affected tests) to catch concept satisfaction regressions +4. Run with **AddressSanitizer** (especially Phase 1) to detect dangling references +5. Run **benchmarks** (especially Phase 3) to verify no performance regression + +## Open Questions + +1. **~~Should `vertex_id_t` parameters use forwarding references instead of `const&`?~~** + **Resolved.** The `reference_wrapper` pattern for internal storage eliminates the main + motivation for forwarding references (avoiding copies into queues/stacks). Parameters use + `const vertex_id_t&`; internal storage uses `vertex_id_store_t`. No forwarding + references needed. + +2. **CPO return type propagation.** If `vertex_descriptor::vertex_id()` returns `decltype(auto)`, + and the CPO `vertex_id(g, u)` uses `-> decltype(auto)`, the CPO will return a reference + for map-based graphs. **This is the desired semantics** — it is what enables + `raw_vertex_id_t` to detect reference-ness and select the `reference_wrapper` path. + The reference is valid as long as the graph is alive, which is guaranteed during algorithm + execution. The `vertex_id_t` alias strips the reference (Phase 0) so code declaring + local value variables still gets copy semantics. + +3. **Default arguments.** Functions like `mis(g, ..., vertex_id_t seed = 0)` use a default + value. With `const vertex_id_t& seed`, the default must be a materializable value: + `const vertex_id_t& seed = vertex_id_t{}` or similar. For algorithms constrained to + `index_adjacency_list`, keeping by-value for defaulted parameters may be pragmatic. + +4. **Map-based property containers for non-integral vertex IDs.** When vertices are keyed by + non-integral types (e.g., `std::string`), every algorithm property container — distances, + predecessors, color maps, etc. — must also become map-based. Dijkstra currently requires + `random_access_range` for `Distances` and `Predecessors` with `distances[static_cast(uid)]` + indexing. For map-based graphs this would become + `std::unordered_map, distance_type>` and + `std::unordered_map, vertex_id_t>` respectively, with `distances[uid]` or + `distances.at(uid)` access. + + This has cascading design implications beyond simple container substitution: + + - **Concept constraints change.** `random_access_range` would need to be relaxed + to something like an associative-container concept or a generic property-map concept (similar + to BGL1's `ReadWritePropertyMap`). A property-map abstraction that unifies `vector`-indexing + and `map`-lookup behind a single `get(map, key)` / `put(map, key, value)` interface would + be the cleanest approach. This is the pattern BGL1 used and it scaled well. + + - **Initialization differs.** `std::vector` is pre-sized and filled with + `infinite` in O(V). An `unordered_map` starts empty; vertices are inserted on first + discovery. The algorithm must handle "key not present" as equivalent to "distance = infinite" + (defaulting behavior), or pre-populate from the vertex range. + + - **Predecessor storage.** For `predecessor[vid] = uid`, if both `predecessor` and the queue + use `vertex_id_store_t` (`reference_wrapper`), predecessor entries are 8-byte wrappers + referencing stable graph keys — efficient. But if predecessors are a result returned to the + caller (outliving the algorithm), they must store owned `vertex_id_t` values, not + `reference_wrapper`s. This means the predecessor map's value type should be `vertex_id_t` + (the value type), even though internal queue elements are `vertex_id_store_t`. The + assignment `predecessor[vid] = uid` where `uid` is a `reference_wrapper` works because + `reference_wrapper` implicitly converts to `const string&`, which can + initialize a `string` value in the map. + + - **Sort-order for priority queue.** `std::priority_queue` with `reference_wrapper` elements + works if the comparator accepts `const vertex_id_t&` (which `reference_wrapper` converts + to). But if the comparator needs to index into distances, it must use the same map-based + lookup: `distances[a.get()]` rather than `distances[static_cast(a)]`. + + **This is out of scope for the current effort** but is the natural next step after the + `const&` / `reference_wrapper` foundation is in place. A suggested approach for that future + work is discussed below, along with a comparison to BGL1's property maps. + + **Property maps vs. function objects for algorithm parameters:** + + BGL1 used a `PropertyMap` concept (`get(pm, key)` / `put(pm, key, value)`) as the universal + abstraction for all algorithm data — distances, predecessors, color maps, edge weights, vertex + indices. This was powerful but widely criticized for its complexity: users had to understand + property map categories (`ReadablePropertyMap`, `WritablePropertyMap`, + `ReadWritePropertyMap`, `LvaluePropertyMap`), tag-dispatch for bundled vs. external properties, + and the `property_traits` machinery. The learning curve was steep even for common cases. + + The modern C++ standard library has established a different pattern: **function objects** + (callables) for customization. `std::sort` takes a comparator. `std::transform` takes a + unary operation. `std::accumulate` takes a binary operation. Graph-v3 already follows this + idiom — Dijkstra takes `WF` (weight function), `Compare`, and `Combine` as callable template + parameters. This is familiar to any C++ developer. + + The question is whether distances, predecessors, and color maps should follow the property-map + pattern or the function-object pattern. Arguments for each: + + **Arguments for a property-map concept (BGL1-style `get`/`put`):** + - Unifies `vector[i]`, `map[key]`, and computed-on-the-fly access behind one interface. + - Natural for read-write properties: `get(dist, uid)` and `put(dist, uid, value)` mirror + the dual-direction access that algorithms actually need. + - Enables static dispatch on property-map category (readable vs. writable vs. lvalue) for + algorithm optimizations. + - Predecessors, distances, and color maps are conceptually "associated data indexed by vertex" + — a property map is the natural abstraction for that. + - If a `_null_predecessors` sentinel is used to skip predecessor tracking, a property-map + concept makes this natural: `null_property_map` satisfies the concept but `put()` is a no-op. + + **Arguments against property maps / for function objects or direct container concepts:** + - **Complexity.** BGL1's property maps are the single most-cited barrier to adoption. Adding a + property-map layer to graph-v3 risks repeating that mistake. + - **Standard practice.** The standard library does not use property maps. Algorithms take + ranges and callables. Developers expect `vector& distances`, not + `WritablePropertyMap distances`. + - **`operator[]` already works.** Both `vector` and `unordered_map` support `operator[]` with + the appropriate key type. A concept requiring `operator[]` (or a subscriptable/indexable + concept) achieves the same unification as `get`/`put` without introducing new vocabulary. + - **Function objects compose better.** A weight function `WF(g, edge) -> weight` can wrap + an edge property, compute on-the-fly, or look up in an external array. No property-map + adapter needed. The same pattern can work for distance access if needed: + `dist_get(uid) -> distance`, `dist_put(uid, value)`. But this is arguably just reinventing + property maps with lambda syntax. + - **Ranges + projections.** C++20 ranges use projections (`std::ranges::sort(r, comp, proj)`) + as the standard way to customize element access. This could be extended to graph properties. + + **Recommendation for graph-v3:** + + A minimal approach that avoids BGL1's complexity while solving the indexing problem: + + 1. **Keep the current approach for the integral path.** `random_access_range` with + `distances[uid]` is simple, efficient, and familiar. No change needed for vector-based + graphs. + 2. **For the non-integral path, require `operator[]` on the container.** Both `vector` and + `unordered_map` support `distances[uid]`. A concept like: + ```cpp + template + concept vertex_property_map = requires(M& m, const Key& k) { + { m[k] } -> std::convertible_to; + }; + ``` + This is not a BGL1-style property map — it is a simple subscriptable concept that both + `vector` and `unordered_map` satisfy naturally. No `get`/`put` free functions, + no property-map categories, no traits machinery. Just `m[k]`. + 3. **`_null_predecessors` remains a special type** that satisfies the concept with no-op + `operator[]` (as it effectively does today via `_null_range_type`). + 4. **Edge weight stays as a function object** (`WF`), unchanged. This is the right abstraction + for "compute a value from an edge" and is consistent with standard library practice. + + This avoids the BGL1 property-map abstraction entirely while still supporting map-based + containers. The key insight is that `operator[]` is already the universal access syntax + that both `vector` and `unordered_map` share — no additional abstraction layer is needed. + +5. **`reference_wrapper` and `std::hash`.** If an algorithm uses `std::unordered_set` for + visited tracking, `std::hash>` is not provided by default. + A custom hasher delegating to `std::hash>` via `.get()` would be needed. + Alternatively, the visited set could use `vertex_id_t` (value type) directly, accepting + the copy cost for set insertion in exchange for simpler hashing. diff --git a/docs/contributing/algorithm-template.md b/docs/contributing/algorithm-template.md index 3af564f..8770886 100644 --- a/docs/contributing/algorithm-template.md +++ b/docs/contributing/algorithm-template.md @@ -94,11 +94,11 @@ template (edge_t)>> requires /* ... constraints ... */ void algorithm_name( - G&& g, // graph - vertex_id_t source, // starting vertex - Distance& distance, // out: distances - Predecessor& predecessor, // out: predecessors - WF&& weight) // edge weight function + G&& g, // graph + const vertex_id_t& source, // starting vertex + Distance& distance, // out: distances + Predecessor& predecessor, // out: predecessors + WF&& weight) // edge weight function ``` --- diff --git a/docs/user-guide/algorithms/bellman_ford.md b/docs/user-guide/algorithms/bellman_ford.md index 6e7e326..3dfa685 100644 --- a/docs/user-guide/algorithms/bellman_ford.md +++ b/docs/user-guide/algorithms/bellman_ford.md @@ -75,7 +75,7 @@ bellman_ford_shortest_paths(G&& g, const Sources& sources, // Single-source, distances + predecessors [[nodiscard]] constexpr optional> -bellman_ford_shortest_paths(G&& g, vertex_id_t source, +bellman_ford_shortest_paths(G&& g, const vertex_id_t& source, Distances& distances, Predecessors& predecessors, WF&& weight, Visitor&& visitor = empty_visitor(), @@ -93,7 +93,7 @@ bellman_ford_shortest_distances(G&& g, const Sources& sources, // Single-source, distances only [[nodiscard]] constexpr optional> -bellman_ford_shortest_distances(G&& g, vertex_id_t source, +bellman_ford_shortest_distances(G&& g, const vertex_id_t& source, Distances& distances, WF&& weight, Visitor&& visitor = empty_visitor(), diff --git a/docs/user-guide/algorithms/bfs.md b/docs/user-guide/algorithms/bfs.md index f681461..1a3177e 100644 --- a/docs/user-guide/algorithms/bfs.md +++ b/docs/user-guide/algorithms/bfs.md @@ -66,7 +66,7 @@ void breadth_first_search(G&& g, const Sources& sources, Visitor&& visitor = empty_visitor()); // Single-source BFS -void breadth_first_search(G&& g, vertex_id_t source, +void breadth_first_search(G&& g, const vertex_id_t& source, Visitor&& visitor = empty_visitor()); ``` diff --git a/docs/user-guide/algorithms/dijkstra.md b/docs/user-guide/algorithms/dijkstra.md index 15e4d91..5e4d053 100644 --- a/docs/user-guide/algorithms/dijkstra.md +++ b/docs/user-guide/algorithms/dijkstra.md @@ -77,7 +77,7 @@ constexpr void dijkstra_shortest_paths(G&& g, const Sources& sources, Combine&& combine = plus<>{}); // Single-source, distances + predecessors -constexpr void dijkstra_shortest_paths(G&& g, vertex_id_t source, +constexpr void dijkstra_shortest_paths(G&& g, const vertex_id_t& source, Distances& distances, Predecessors& predecessors, WF&& weight = /* default returns 1 */, Visitor&& visitor = empty_visitor(), @@ -93,7 +93,7 @@ constexpr void dijkstra_shortest_distances(G&& g, const Sources& sources, Combine&& combine = plus<>{}); // Single-source, distances only -constexpr void dijkstra_shortest_distances(G&& g, vertex_id_t source, +constexpr void dijkstra_shortest_distances(G&& g, const vertex_id_t& source, Distances& distances, WF&& weight = /* default returns 1 */, Visitor&& visitor = empty_visitor(), diff --git a/docs/user-guide/algorithms/mis.md b/docs/user-guide/algorithms/mis.md index 5d48f08..644d955 100644 --- a/docs/user-guide/algorithms/mis.md +++ b/docs/user-guide/algorithms/mis.md @@ -67,7 +67,7 @@ remaining unmarked vertices in order. ```cpp size_t maximal_independent_set(G&& g, OutputIterator mis, - vertex_id_t seed = 0); + const vertex_id_t& seed = 0); ``` **Returns** the number of vertices in the MIS. Selected vertex IDs are written diff --git a/docs/user-guide/algorithms/mst.md b/docs/user-guide/algorithms/mst.md index 624c37b..47f30ae 100644 --- a/docs/user-guide/algorithms/mst.md +++ b/docs/user-guide/algorithms/mst.md @@ -109,12 +109,12 @@ lightest edge crossing the cut between tree and non-tree vertices. ```cpp // Simple: uses default weight function from edge values auto prim(G&& g, Predecessors& predecessors, Weights& weights, - vertex_id_t seed = 0); + const vertex_id_t& seed = 0); // Full: custom comparator, initial distance, and weight function auto prim(G&& g, Predecessors& predecessors, Weights& weights, Compare compare, range_value_t init_dist, - WF weight_fn, vertex_id_t seed = 0); + WF weight_fn, const vertex_id_t& seed = 0); ``` **Returns** the total MST weight. diff --git a/docs/user-guide/algorithms/topological_sort.md b/docs/user-guide/algorithms/topological_sort.md index 8eb6474..0ddf049 100644 --- a/docs/user-guide/algorithms/topological_sort.md +++ b/docs/user-guide/algorithms/topological_sort.md @@ -73,7 +73,7 @@ Three overloads cover different use cases: bool topological_sort(const G& g, OutputIterator result); // Single-source topological sort -bool topological_sort(const G& g, vertex_id_t source, OutputIterator result); +bool topological_sort(const G& g, const vertex_id_t& source, OutputIterator result); // Multi-source topological sort bool topological_sort(const G& g, const Sources& sources, OutputIterator result); diff --git a/examples/dijkstra_clrs.hpp b/examples/dijkstra_clrs.hpp index 7cf1379..ddfa45e 100644 --- a/examples/dijkstra_clrs.hpp +++ b/examples/dijkstra_clrs.hpp @@ -87,8 +87,8 @@ requires forward_range> && // convertible_to, range_value_t> && // edge_weight_function void dijkstra_clrs( - G&& g, // graph - vertex_id_t seed, // starting vertex_id + G&& g, // graph + const vertex_id_t& seed, // starting vertex_id Distance& distance, // out: distance[uid] of uid from seed Predecessor& predecessor, // out: predecessor[uid] of uid in path WF&& weight = [](const auto&, diff --git a/include/graph/adj_list/adjacency_list_concepts.hpp b/include/graph/adj_list/adjacency_list_concepts.hpp index b30e706..52f8d58 100644 --- a/include/graph/adj_list/adjacency_list_concepts.hpp +++ b/include/graph/adj_list/adjacency_list_concepts.hpp @@ -100,7 +100,7 @@ concept out_edge_range = std::ranges::forward_range && edge -concept vertex = is_vertex_descriptor_v> && requires(G& g, const V& u, vertex_id_t uid) { +concept vertex = is_vertex_descriptor_v> && requires(G& g, const V& u, const vertex_id_t& uid) { vertex_id(g, u); find_vertex(g, uid); }; diff --git a/include/graph/adj_list/adjacency_list_traits.hpp b/include/graph/adj_list/adjacency_list_traits.hpp index a27bab8..67af999 100644 --- a/include/graph/adj_list/adjacency_list_traits.hpp +++ b/include/graph/adj_list/adjacency_list_traits.hpp @@ -29,7 +29,7 @@ namespace detail { // Helper to detect if degree(g, uid) is valid template - concept has_degree_uid_impl = requires(G& g, vertex_id_t uid) { + concept has_degree_uid_impl = requires(G& g, const vertex_id_t& uid) { { degree(g, uid) } -> std::integral; }; } // namespace detail @@ -59,7 +59,7 @@ inline constexpr bool has_degree_v = has_degree; namespace detail { // Helper to detect if find_vertex(g, uid) is valid and returns correct type template - concept has_find_vertex_impl = requires(G& g, vertex_id_t uid) { + concept has_find_vertex_impl = requires(G& g, const vertex_id_t& uid) { { find_vertex(g, uid) } -> std::same_as>; }; } // namespace detail @@ -94,13 +94,13 @@ namespace detail { // Helper to detect if find_vertex_edge(g, u, vid) is valid template - concept has_find_vertex_edge_uvid_impl = requires(G& g, vertex_t u, vertex_id_t vid) { + concept has_find_vertex_edge_uvid_impl = requires(G& g, vertex_t u, const vertex_id_t& vid) { { find_vertex_edge(g, u, vid) } -> std::same_as>; }; // Helper to detect if find_vertex_edge(g, uid, vid) is valid template - concept has_find_vertex_edge_uidvid_impl = requires(G& g, vertex_id_t uid, vertex_id_t vid) { + concept has_find_vertex_edge_uidvid_impl = requires(G& g, const vertex_id_t& uid, const vertex_id_t& vid) { { find_vertex_edge(g, uid, vid) } -> std::same_as>; }; } // namespace detail @@ -206,13 +206,13 @@ namespace detail { // Helper to detect if find_in_edge(g, u, vid) is valid with descriptor + id template - concept has_find_in_edge_uid_impl = requires(G& g, vertex_t u, vertex_id_t vid) { + concept has_find_in_edge_uid_impl = requires(G& g, vertex_t u, const vertex_id_t& vid) { { find_in_edge(g, u, vid) }; }; // Helper to detect if find_in_edge(g, uid, vid) is valid with vertex IDs template - concept has_find_in_edge_uidvid_impl = requires(G& g, vertex_id_t uid, vertex_id_t vid) { + concept has_find_in_edge_uidvid_impl = requires(G& g, const vertex_id_t& uid, const vertex_id_t& vid) { { find_in_edge(g, uid, vid) }; }; } // namespace detail diff --git a/include/graph/adj_list/detail/graph_cpo.hpp b/include/graph/adj_list/detail/graph_cpo.hpp index e21fc6d..d3ce56f 100644 --- a/include/graph/adj_list/detail/graph_cpo.hpp +++ b/include/graph/adj_list/detail/graph_cpo.hpp @@ -388,13 +388,24 @@ inline namespace _cpo_instances { /** * @brief Vertex ID type for graph G * - * The type of the unique identifier returned by vertex_id(g, u). + * The type of the unique identifier returned by vertex_id(g, u), with references stripped. * - For random-access containers (vector, deque): size_t (index) - * - For associative containers (map): key type + * - For associative containers (map): key type (e.g. std::string) * - For bidirectional containers: iterator-based ID */ template -using vertex_id_t = decltype(vertex_id(std::declval(), std::declval>())); +using vertex_id_t = std::remove_cvref_t< + decltype(vertex_id(std::declval(), std::declval>()))>; + +/** + * @brief Raw return type of vertex_id(g, u), preserving reference-ness. + * + * For vector-based graphs this is a prvalue (e.g. size_t). + * For map-based graphs this is an lvalue reference to the stable key (e.g. const std::string&). + * Used primarily by vertex_id_store_t to select the optimal internal storage strategy. + */ +template +using raw_vertex_id_t = decltype(vertex_id(std::declval(), std::declval>())); namespace _cpo_impls { diff --git a/include/graph/adj_list/edge_descriptor.hpp b/include/graph/adj_list/edge_descriptor.hpp index ae9de25..7473f1a 100644 --- a/include/graph/adj_list/edge_descriptor.hpp +++ b/include/graph/adj_list/edge_descriptor.hpp @@ -66,44 +66,33 @@ class edge_descriptor { /** * @brief Get the source vertex ID - * @return The vertex ID of the source vertex + * @return The vertex ID of the source vertex (by value for integral, const& for map keys) * * Extracts the ID from the stored source vertex descriptor. - * - * @note Current implementation returns by value (auto) which is suitable for - * integral vertex IDs. When non-trivial vertex ID types (e.g., std::string) - * are supported, this method should: - * 1. Change return type to decltype(auto) for reference semantics - * 2. This delegates to vertex_descriptor::vertex_id() which will also need updating - * See descriptor.md "Lambda Reference Binding Issues" section for details. + * Returns decltype(auto) to propagate reference semantics from vertex_descriptor::vertex_id(). */ - [[nodiscard]] constexpr auto source_id() const noexcept { return source_.vertex_id(); } + [[nodiscard]] constexpr decltype(auto) source_id() const noexcept { return source_.vertex_id(); } /** * @brief Get the target vertex ID from the edge data * @param vertex_data The vertex/edge data structure passed from the CPO * @return The target vertex identifier extracted from the edge * - * For random access iterators, uses the stored index to access the container. - * For forward/bidirectional iterators, dereferences the stored iterator. - * - * The vertex_data parameter varies by graph structure: - * - vov-style: vertex object with .edges() method - * - map-based: std::pair where .second is the vertex - * - raw adjacency: the edge container directly + * Returns decltype(auto) for reference semantics on non-trivial ID types. + * Pair-like .first branches are parenthesized to return const& to the stable + * key rather than copying. std::get<0> and native .target_id() calls already + * return appropriate types. + */ + /** + * @brief Get the source vertex ID by navigating the edge container (in-edge only) + * @param vertex_data The vertex/edge data structure passed from the CPO + * @return The source vertex identifier extracted from the native in-edge * - * Edge data extraction: - * - Simple integral types: returns the value directly as target ID - * - Pair-like types: returns .first as target ID - * - Tuple-like types: returns std::get<0> as target ID + * For in-edge descriptors, source_id() (no args) returns the owning vertex ID + * which is actually the target. This overload navigates the in_edges container + * to retrieve the native edge's source_id — the actual source vertex. * - * @note Current implementation returns by value (auto) which is suitable for - * integral vertex IDs. When non-trivial vertex ID types (e.g., std::string) - * are supported, this method should: - * 1. Change return type to decltype(auto) for reference semantics - * 2. Replace lambda-based extraction with direct if constexpr branches - * 3. Wrap return expressions in parentheses: return (edge_val.first); - * See descriptor.md "Lambda Reference Binding Issues" section for details. + * Only available when EdgeDirection is in_edge_tag. */ /** * @brief Get the source vertex ID by navigating the edge container (in-edge only) @@ -206,7 +195,7 @@ class edge_descriptor { if constexpr (requires { vertex_data.second.edges(); }) { return vertex_data.second.edges(); } else { - return vertex_data.second; + return (vertex_data.second); // parenthesized: return const& to the container } } else { return vertex_data; @@ -234,8 +223,8 @@ class edge_descriptor { // Edge object with target_id() member (e.g., dynamic_out_edge) return edge_val.target_id(); } else if constexpr (requires { edge_val.first; }) { - // Pair-like: .first is the target ID - return edge_val.first; + // Pair-like: .first is the target ID — parenthesized for reference semantics + return (edge_val.first); } else if constexpr (requires { std::get<0>(edge_val); }) { // Tuple-like: first element is the target ID return std::get<0>(edge_val); diff --git a/include/graph/adj_list/vertex_descriptor.hpp b/include/graph/adj_list/vertex_descriptor.hpp index 71fb169..b97568d 100644 --- a/include/graph/adj_list/vertex_descriptor.hpp +++ b/include/graph/adj_list/vertex_descriptor.hpp @@ -45,16 +45,18 @@ class vertex_descriptor { /** * @brief Get the vertex ID - * @return For random access: the index. For bidirectional: the key from the pair + * @return For random access: the index (by value). For bidirectional: const& to the key. * - * @note Current implementation returns by value (auto) which is suitable for - * integral vertex IDs. When non-trivial vertex ID types (e.g., std::string) - * are supported, this method should: - * 1. Change return type to decltype(auto) for reference semantics - * 2. Wrap return expressions in parentheses: return (storage_); - * See descriptor.md "Lambda Reference Binding Issues" section for details. + * Returns decltype(auto) so that map-based graphs return a const reference + * to the stable key stored in the map node, avoiding copies of non-trivial + * ID types like std::string. + * + * For random-access iterators, `return storage_;` (non-parenthesized) ensures + * decltype deduces the value type (size_t) — safe even on temporaries. + * For bidirectional iterators, std::get<0> returns a reference to the map key, + * which is stable as long as the container is alive. */ - [[nodiscard]] constexpr auto vertex_id() const noexcept { + [[nodiscard]] constexpr decltype(auto) vertex_id() const noexcept { if constexpr (std::random_access_iterator) { return storage_; } else { diff --git a/include/graph/algorithm/bellman_ford_shortest_paths.hpp b/include/graph/algorithm/bellman_ford_shortest_paths.hpp index 36702c0..d6a6ecc 100644 --- a/include/graph/algorithm/bellman_ford_shortest_paths.hpp +++ b/include/graph/algorithm/bellman_ford_shortest_paths.hpp @@ -179,14 +179,16 @@ requires convertible_to, vertex_id_t> && // Visitor&& visitor = empty_visitor(), Compare&& compare = less>(), Combine&& combine = plus>()) { - using id_type = vertex_id_t; + using id_type = vertex_id_store_t; + static_assert(std::is_same_v>, + "vertex_id_store_t should equal vertex_id_t for index_adjacency_list"); using DistanceValue = range_value_t; using weight_type = invoke_result_t&, edge_t>; using return_type = optional>; // relaxing the target is the function of reducing the distance from the source to the target auto relax_target = [&g, &predecessor, &distances, &compare, &combine] // - (const edge_t& e, vertex_id_t uid, const weight_type& w_e) -> bool { + (const edge_t& e, const vertex_id_t& uid, const weight_type& w_e) -> bool { id_type vid = target_id(g, e); const DistanceValue d_u = distances[static_cast(uid)]; const DistanceValue d_v = distances[static_cast(vid)]; @@ -301,10 +303,10 @@ requires is_arithmetic_v> && // sized_range && // basic_edge_weight_function, Compare, Combine> [[nodiscard]] constexpr optional> bellman_ford_shortest_paths( - G&& g, - vertex_id_t source, - Distances& distances, - Predecessors& predecessor, + G&& g, + const vertex_id_t& source, + Distances& distances, + Predecessors& predecessor, WF&& weight = [](const auto&, const edge_t& uv) { return range_value_t(1); }, // default weight(g, uv) -> 1 Visitor&& visitor = empty_visitor(), @@ -392,9 +394,9 @@ requires is_arithmetic_v> && // sized_range && // basic_edge_weight_function, Compare, Combine> [[nodiscard]] constexpr optional> bellman_ford_shortest_distances( - G&& g, - vertex_id_t source, - Distances& distances, + G&& g, + const vertex_id_t& source, + Distances& distances, WF&& weight = [](const auto&, const edge_t& uv) { return range_value_t(1); }, // default weight(g, uv) -> 1 Visitor&& visitor = empty_visitor(), diff --git a/include/graph/algorithm/breadth_first_search.hpp b/include/graph/algorithm/breadth_first_search.hpp index 7b8e63d..c9bed55 100644 --- a/include/graph/algorithm/breadth_first_search.hpp +++ b/include/graph/algorithm/breadth_first_search.hpp @@ -210,7 +210,9 @@ requires std::convertible_to, vertex_id_t void breadth_first_search(G&& g, // graph const Sources& sources, Visitor&& visitor = empty_visitor()) { - using id_type = vertex_id_t; + using id_type = vertex_id_store_t; + static_assert(std::is_same_v>, + "vertex_id_store_t should equal vertex_id_t for index_adjacency_list"); // Initialize BFS data structures std::queue Q; // FIFO queue for level-order traversal @@ -333,9 +335,9 @@ void breadth_first_search(G&& g, // graph * @see views::vertices_bfs BFS view for range-based traversal */ template -void breadth_first_search(G&& g, // graph - vertex_id_t source, // starting vertex_id - Visitor&& visitor = empty_visitor()) { +void breadth_first_search(G&& g, // graph + const vertex_id_t& source, // starting vertex_id + Visitor&& visitor = empty_visitor()) { // Wrap single source in array and delegate to multi-source version std::array, 1> sources{source}; breadth_first_search(std::forward(g), sources, std::forward(visitor)); diff --git a/include/graph/algorithm/connected_components.hpp b/include/graph/algorithm/connected_components.hpp index a7a822c..9ab3a47 100644 --- a/include/graph/algorithm/connected_components.hpp +++ b/include/graph/algorithm/connected_components.hpp @@ -151,7 +151,7 @@ void kosaraju(G&& g, // graph // Helper: iterative DFS to compute finish times (post-order) // This creates reverse topological ordering for SCC discovery - auto dfs_finish_order = [&](vertex_id_t start) { + auto dfs_finish_order = [&](const vertex_id_t& start) { std::stack, bool>> stack; // (vertex, children_visited) stack.push({start, false}); visited[start] = true; @@ -280,7 +280,7 @@ void kosaraju(G&& g, // bidirectional graph auto& g_ref = g; // First pass: iterative DFS to compute finish times (same as two-graph version) - auto dfs_finish_order = [&](vertex_id_t start) { + auto dfs_finish_order = [&](const vertex_id_t& start) { std::stack, bool>> stack; stack.push({start, false}); visited[start] = true; diff --git a/include/graph/algorithm/depth_first_search.hpp b/include/graph/algorithm/depth_first_search.hpp index 60cf7e8..2057eeb 100644 --- a/include/graph/algorithm/depth_first_search.hpp +++ b/include/graph/algorithm/depth_first_search.hpp @@ -242,7 +242,9 @@ template void depth_first_search(G&& g, // graph const vertex_id_t& source, // starting vertex_id Visitor&& visitor = empty_visitor()) { - using id_type = vertex_id_t; + using id_type = vertex_id_store_t; + static_assert(std::is_same_v>, + "vertex_id_store_t should equal vertex_id_t for index_adjacency_list"); // Vertex color states for DFS enum class Color : uint8_t { diff --git a/include/graph/algorithm/dijkstra_shortest_paths.hpp b/include/graph/algorithm/dijkstra_shortest_paths.hpp index d1c276a..ede2b58 100644 --- a/include/graph/algorithm/dijkstra_shortest_paths.hpp +++ b/include/graph/algorithm/dijkstra_shortest_paths.hpp @@ -147,13 +147,15 @@ constexpr void dijkstra_shortest_paths( Visitor&& visitor = empty_visitor(), Compare&& compare = less>(), Combine&& combine = plus>()) { - using id_type = vertex_id_t; + using id_type = vertex_id_store_t; + static_assert(std::is_same_v>, + "vertex_id_store_t should equal vertex_id_t for index_adjacency_list"); using distance_type = range_value_t; using weight_type = invoke_result_t&, edge_t>; // relaxing the target is the function of reducing the distance from the source to the target auto relax_target = [&g, &predecessor, &distances, &compare, &combine] // - (const edge_t& e, vertex_id_t uid, const weight_type& w_e) -> bool { + (const edge_t& e, const vertex_id_t& uid, const weight_type& w_e) -> bool { const id_type vid = target_id(g, e); const distance_type d_u = distances[static_cast(uid)]; const distance_type d_v = distances[static_cast(vid)]; @@ -309,10 +311,10 @@ requires is_arithmetic_v> && // convertible_to, range_value_t> && basic_edge_weight_function, Compare, Combine> constexpr void dijkstra_shortest_paths( - G&& g, - vertex_id_t source, - Distances& distances, - Predecessors& predecessor, + G&& g, + const vertex_id_t& source, + Distances& distances, + Predecessors& predecessor, WF&& weight = [](const auto&, const edge_t& uv) { return range_value_t(1); }, // default weight(g, uv) -> 1 Visitor&& visitor = empty_visitor(), @@ -396,9 +398,9 @@ requires is_arithmetic_v> && // sized_range && // basic_edge_weight_function, Compare, Combine> constexpr void dijkstra_shortest_distances( - G&& g, - vertex_id_t source, - Distances& distances, + G&& g, + const vertex_id_t& source, + Distances& distances, WF&& weight = [](const auto&, const edge_t& uv) { return range_value_t(1); }, // default weight(g, uv) -> 1 Visitor&& visitor = empty_visitor(), diff --git a/include/graph/algorithm/mis.hpp b/include/graph/algorithm/mis.hpp index 2d6f94c..960d0b2 100755 --- a/include/graph/algorithm/mis.hpp +++ b/include/graph/algorithm/mis.hpp @@ -145,9 +145,9 @@ using adj_list::num_vertices; template requires output_iterator> -size_t maximal_independent_set(G&& g, // graph - Iter mis, // out: maximal independent set - vertex_id_t seed = 0 // seed vtx +size_t maximal_independent_set(G&& g, // graph + Iter mis, // out: maximal independent set + const vertex_id_t& seed = 0 // seed vtx ) { size_t N = num_vertices(g); if (N == 0) { diff --git a/include/graph/algorithm/mst.hpp b/include/graph/algorithm/mst.hpp index c401167..302fdd4 100644 --- a/include/graph/algorithm/mst.hpp +++ b/include/graph/algorithm/mst.hpp @@ -835,7 +835,7 @@ template seed = 0 // seed vtx + const vertex_id_t& seed = 0 // seed vtx ) { // Default weight function: use edge_value CPO auto weight_fn = [](const auto& g, const edge_t& uv) -> range_value_t { return edge_value(g, uv); }; diff --git a/include/graph/algorithm/topological_sort.hpp b/include/graph/algorithm/topological_sort.hpp index d7cbe59..7c4137a 100644 --- a/include/graph/algorithm/topological_sort.hpp +++ b/include/graph/algorithm/topological_sort.hpp @@ -129,6 +129,7 @@ #include "graph/graph.hpp" #include "graph/algorithm/depth_first_search.hpp" +#include "graph/algorithm/traversal_common.hpp" #include "graph/views/incidence.hpp" #include @@ -164,12 +165,14 @@ namespace detail { */ template void topological_sort_dfs_visit(const G& g, - vertex_id_t source, + const vertex_id_t& source, std::vector& color, std::vector>& finish_order, bool& has_cycle) { - using id_type = vertex_id_t; + using id_type = vertex_id_store_t; + static_assert(std::is_same_v>, + "vertex_id_store_t should equal vertex_id_t for index_adjacency_list"); using namespace graph::views; using inc_range_t = decltype(basic_incidence(g, source)); using inc_iterator_t = std::ranges::iterator_t; @@ -334,7 +337,9 @@ template , vertex_id_t> && std::output_iterator> bool topological_sort(const G& g, const Sources& sources, OutputIterator result) { - using id_type = vertex_id_t; + using id_type = vertex_id_store_t; + static_assert(std::is_same_v>, + "vertex_id_store_t should equal vertex_id_t for index_adjacency_list"); // Vertex color states for DFS enum class Color : uint8_t { @@ -457,7 +462,7 @@ bool topological_sort(const G& g, const Sources& sources, OutputIterator result) */ template requires std::output_iterator> -bool topological_sort(const G& g, vertex_id_t source, OutputIterator result) { +bool topological_sort(const G& g, const vertex_id_t& source, OutputIterator result) { // Delegate to multi-source version with single source std::array, 1> sources = {source}; return topological_sort(g, sources, result); @@ -544,7 +549,9 @@ bool topological_sort(const G& g, vertex_id_t source, OutputIterator result) template requires std::output_iterator> bool topological_sort(const G& g, OutputIterator result) { - using id_type = vertex_id_t; + using id_type = vertex_id_store_t; + static_assert(std::is_same_v>, + "vertex_id_store_t should equal vertex_id_t for index_adjacency_list"); // Vertex color states for DFS enum class Color : uint8_t { diff --git a/include/graph/algorithm/traversal_common.hpp b/include/graph/algorithm/traversal_common.hpp index fdd9f71..33daaf3 100644 --- a/include/graph/algorithm/traversal_common.hpp +++ b/include/graph/algorithm/traversal_common.hpp @@ -29,6 +29,26 @@ namespace graph { +// +// vertex_id_store_t — efficient storage type for vertex IDs in algorithm internals +// + +/** + * @brief Efficient storage type for vertex IDs in algorithm-internal containers. + * + * For integral IDs (vector-based graphs): same as vertex_id_t (zero overhead). + * For non-trivial IDs (e.g., std::string from map-based graphs): reference_wrapper + * to the stable key in the graph's map node (8 bytes, trivially copyable). + * + * @note Requires: the graph must not be mutated while values of this type are alive. + * This is already an implicit precondition for all traversal algorithms. + */ +template +using vertex_id_store_t = std::conditional_t< + std::is_reference_v>, + std::reference_wrapper>>, + adj_list::vertex_id_t>; + // // Edge weight function concepts // diff --git a/include/graph/edge_list/edge_list.hpp b/include/graph/edge_list/edge_list.hpp index 0e90922..8ebe427 100644 --- a/include/graph/edge_list/edge_list.hpp +++ b/include/graph/edge_list/edge_list.hpp @@ -140,6 +140,11 @@ namespace edge_list { using vertex_id_t = std::remove_cvref_t&>(), std::declval>>()))>; + /// Raw return type of source_id(el, uv), preserving reference-ness. + template // For exposition only + using raw_vertex_id_t = decltype(graph::source_id(std::declval&>(), + std::declval>>())); + // template aliases can't be distinguished with concepts :( // diff --git a/include/graph/views/bfs.hpp b/include/graph/views/bfs.hpp index 6ba95e1..bacf0d8 100644 --- a/include/graph/views/bfs.hpp +++ b/include/graph/views/bfs.hpp @@ -353,7 +353,7 @@ class vertices_bfs_view : public std::ranges::view_int : g_(&g), state_(std::make_shared(g, seed_vertex, adj_list::num_vertices(g), alloc)) {} /// Construct from vertex ID (delegates to vertex descriptor constructor) - vertices_bfs_view(G& g, vertex_id_type seed, Alloc alloc = {}) + vertices_bfs_view(G& g, const vertex_id_type& seed, Alloc alloc = {}) : vertices_bfs_view(g, *adj_list::find_vertex(g, seed), alloc) {} [[nodiscard]] iterator begin() { return iterator(g_, state_); } @@ -507,7 +507,7 @@ class vertices_bfs_view : public std::ranges::view_interface(g, seed_vertex, adj_list::num_vertices(g), alloc)) {} /// Construct from vertex ID (delegates to vertex descriptor constructor) - vertices_bfs_view(G& g, vertex_id_type seed, VVF vvf, Alloc alloc = {}) + vertices_bfs_view(G& g, const vertex_id_type& seed, VVF vvf, Alloc alloc = {}) : vertices_bfs_view(g, *adj_list::find_vertex(g, seed), std::move(vvf), alloc) {} [[nodiscard]] iterator begin() { return iterator(g_, state_, &vvf_); } @@ -534,13 +534,13 @@ class vertices_bfs_view : public std::ranges::view_interface> -vertices_bfs_view(G&, adj_list::vertex_id_t, Alloc) -> vertices_bfs_view; +vertices_bfs_view(G&, const adj_list::vertex_id_t&, Alloc) -> vertices_bfs_view; template -vertices_bfs_view(G&, adj_list::vertex_id_t) -> vertices_bfs_view>; +vertices_bfs_view(G&, const adj_list::vertex_id_t&) -> vertices_bfs_view>; template > -vertices_bfs_view(G&, adj_list::vertex_id_t, VVF, Alloc) -> vertices_bfs_view; +vertices_bfs_view(G&, const adj_list::vertex_id_t&, VVF, Alloc) -> vertices_bfs_view; // Deduction guides for vertex descriptor template > @@ -561,7 +561,7 @@ vertices_bfs_view(G&, adj_list::vertex_t, VVF, Alloc) -> vertices_bfs_view -[[nodiscard]] auto vertices_bfs(G& g, adj_list::vertex_id_t seed) { +[[nodiscard]] auto vertices_bfs(G& g, const adj_list::vertex_id_t& seed) { return vertices_bfs_view>(g, seed, std::allocator{}); } @@ -590,7 +590,7 @@ template */ template requires vertex_value_function> -[[nodiscard]] auto vertices_bfs(G& g, adj_list::vertex_id_t seed, VVF&& vvf) { +[[nodiscard]] auto vertices_bfs(G& g, const adj_list::vertex_id_t& seed, VVF&& vvf) { return vertices_bfs_view, std::allocator>(g, seed, std::forward(vvf), std::allocator{}); } @@ -624,7 +624,7 @@ requires vertex_value_function> */ template requires(!vertex_value_function>) -[[nodiscard]] auto vertices_bfs(G& g, adj_list::vertex_id_t seed, Alloc alloc) { +[[nodiscard]] auto vertices_bfs(G& g, const adj_list::vertex_id_t& seed, Alloc alloc) { return vertices_bfs_view(g, seed, alloc); } @@ -658,7 +658,7 @@ requires(!vertex_value_function>) */ template requires vertex_value_function> -[[nodiscard]] auto vertices_bfs(G& g, adj_list::vertex_id_t seed, VVF&& vvf, Alloc alloc) { +[[nodiscard]] auto vertices_bfs(G& g, const adj_list::vertex_id_t& seed, VVF&& vvf, Alloc alloc) { return vertices_bfs_view, Alloc>(g, seed, std::forward(vvf), alloc); } @@ -840,7 +840,7 @@ class edges_bfs_view : public std::ranges::view_interf : g_(&g), state_(std::make_shared(g, seed_vertex, adj_list::num_vertices(g), alloc)) {} /// Construct from vertex ID (delegates to vertex descriptor constructor) - edges_bfs_view(G& g, vertex_id_type seed, Alloc alloc = {}) + edges_bfs_view(G& g, const vertex_id_type& seed, Alloc alloc = {}) : edges_bfs_view(g, *adj_list::find_vertex(g, seed), alloc) {} [[nodiscard]] iterator begin() { return iterator(g_, state_); } @@ -1030,7 +1030,7 @@ class edges_bfs_view : public std::ranges::view_interface(g, seed_vertex, adj_list::num_vertices(g), alloc)) {} /// Construct from vertex ID (delegates to vertex descriptor constructor) - edges_bfs_view(G& g, vertex_id_type seed, EVF evf, Alloc alloc = {}) + edges_bfs_view(G& g, const vertex_id_type& seed, EVF evf, Alloc alloc = {}) : edges_bfs_view(g, *adj_list::find_vertex(g, seed), std::move(evf), alloc) {} [[nodiscard]] iterator begin() { return iterator(g_, state_, &evf_); } @@ -1057,13 +1057,13 @@ class edges_bfs_view : public std::ranges::view_interface -edges_bfs_view(G&, adj_list::vertex_id_t, Alloc) -> edges_bfs_view; +edges_bfs_view(G&, const adj_list::vertex_id_t&, Alloc) -> edges_bfs_view; template edges_bfs_view(G&, adj_list::vertex_t, Alloc) -> edges_bfs_view; template -edges_bfs_view(G&, adj_list::vertex_id_t, EVF, Alloc) -> edges_bfs_view; +edges_bfs_view(G&, const adj_list::vertex_id_t&, EVF, Alloc) -> edges_bfs_view; template edges_bfs_view(G&, adj_list::vertex_t, EVF, Alloc) -> edges_bfs_view; @@ -1081,7 +1081,7 @@ edges_bfs_view(G&, adj_list::vertex_t, EVF, Alloc) -> edges_bfs_view -[[nodiscard]] auto edges_bfs(G& g, adj_list::vertex_id_t seed) { +[[nodiscard]] auto edges_bfs(G& g, const adj_list::vertex_id_t& seed) { return edges_bfs_view>(g, seed); } @@ -1110,7 +1110,7 @@ template */ template requires edge_value_function> && (!std::is_same_v, std::allocator>) -[[nodiscard]] auto edges_bfs(G& g, adj_list::vertex_id_t seed, EVF&& evf) { +[[nodiscard]] auto edges_bfs(G& g, const adj_list::vertex_id_t& seed, EVF&& evf) { return edges_bfs_view, std::allocator>(g, seed, std::forward(evf)); } @@ -1142,7 +1142,7 @@ requires edge_value_function> && (!std::is_same_v requires(!edge_value_function>) -[[nodiscard]] auto edges_bfs(G& g, adj_list::vertex_id_t seed, Alloc alloc) { +[[nodiscard]] auto edges_bfs(G& g, const adj_list::vertex_id_t& seed, Alloc alloc) { return edges_bfs_view(g, seed, alloc); } @@ -1176,7 +1176,7 @@ requires(!edge_value_function>) */ template requires edge_value_function> -[[nodiscard]] auto edges_bfs(G& g, adj_list::vertex_id_t seed, EVF&& evf, Alloc alloc) { +[[nodiscard]] auto edges_bfs(G& g, const adj_list::vertex_id_t& seed, EVF&& evf, Alloc alloc) { return edges_bfs_view, Alloc>(g, seed, std::forward(evf), alloc); } @@ -1206,7 +1206,7 @@ requires edge_value_function> /// BFS vertex traversal with explicit Accessor, from vertex ID. template -[[nodiscard]] auto vertices_bfs(G& g, adj_list::vertex_id_t seed) { +[[nodiscard]] auto vertices_bfs(G& g, const adj_list::vertex_id_t& seed) { return vertices_bfs_view, Accessor>(g, seed, std::allocator{}); } @@ -1219,7 +1219,7 @@ template /// BFS vertex traversal with explicit Accessor and value function, from vertex ID. template requires vertex_value_function> -[[nodiscard]] auto vertices_bfs(G& g, adj_list::vertex_id_t seed, VVF&& vvf) { +[[nodiscard]] auto vertices_bfs(G& g, const adj_list::vertex_id_t& seed, VVF&& vvf) { return vertices_bfs_view, std::allocator, Accessor>(g, seed, std::forward(vvf), std::allocator{}); } @@ -1235,7 +1235,7 @@ requires vertex_value_function> /// BFS edge traversal with explicit Accessor, from vertex ID. template -[[nodiscard]] auto edges_bfs(G& g, adj_list::vertex_id_t seed) { +[[nodiscard]] auto edges_bfs(G& g, const adj_list::vertex_id_t& seed) { return edges_bfs_view, Accessor>(g, seed); } @@ -1248,7 +1248,7 @@ template /// BFS edge traversal with explicit Accessor and value function, from vertex ID. template requires edge_value_function> -[[nodiscard]] auto edges_bfs(G& g, adj_list::vertex_id_t seed, EVF&& evf) { +[[nodiscard]] auto edges_bfs(G& g, const adj_list::vertex_id_t& seed, EVF&& evf) { return edges_bfs_view, std::allocator, Accessor>(g, seed, std::forward(evf)); } diff --git a/include/graph/views/dfs.hpp b/include/graph/views/dfs.hpp index 5ca16d5..2ad529c 100644 --- a/include/graph/views/dfs.hpp +++ b/include/graph/views/dfs.hpp @@ -343,7 +343,7 @@ class vertices_dfs_view : public std::ranges::view_int : g_(&g), state_(std::make_shared(g, seed_vertex, adj_list::num_vertices(g), alloc)) {} /// Construct from vertex ID (delegates to vertex descriptor constructor) - vertices_dfs_view(G& g, vertex_id_type seed, Alloc alloc = {}) + vertices_dfs_view(G& g, const vertex_id_type& seed, Alloc alloc = {}) : vertices_dfs_view(g, *adj_list::find_vertex(g, seed), alloc) {} [[nodiscard]] iterator begin() { return iterator(g_, state_); } @@ -519,7 +519,7 @@ class vertices_dfs_view : public std::ranges::view_interface(g, seed_vertex, adj_list::num_vertices(g), alloc)) {} /// Construct from vertex ID (delegates to vertex descriptor constructor) - vertices_dfs_view(G& g, vertex_id_type seed, VVF vvf, Alloc alloc = {}) + vertices_dfs_view(G& g, const vertex_id_type& seed, VVF vvf, Alloc alloc = {}) : vertices_dfs_view(g, *adj_list::find_vertex(g, seed), std::move(vvf), alloc) {} [[nodiscard]] iterator begin() { return iterator(g_, state_, &vvf_); } @@ -546,13 +546,13 @@ class vertices_dfs_view : public std::ranges::view_interface> -vertices_dfs_view(G&, adj_list::vertex_id_t, Alloc) -> vertices_dfs_view; +vertices_dfs_view(G&, const adj_list::vertex_id_t&, Alloc) -> vertices_dfs_view; template -vertices_dfs_view(G&, adj_list::vertex_id_t) -> vertices_dfs_view>; +vertices_dfs_view(G&, const adj_list::vertex_id_t&) -> vertices_dfs_view>; template > -vertices_dfs_view(G&, adj_list::vertex_id_t, VVF, Alloc) -> vertices_dfs_view; +vertices_dfs_view(G&, const adj_list::vertex_id_t&, VVF, Alloc) -> vertices_dfs_view; // Deduction guides for vertex descriptor template > @@ -580,7 +580,7 @@ vertices_dfs_view(G&, adj_list::vertex_t, VVF, Alloc) -> vertices_dfs_view -[[nodiscard]] auto vertices_dfs(G& g, adj_list::vertex_id_t seed) { +[[nodiscard]] auto vertices_dfs(G& g, const adj_list::vertex_id_t& seed) { return vertices_dfs_view>(g, seed, std::allocator{}); } @@ -624,7 +624,7 @@ template */ template requires vertex_value_function> -[[nodiscard]] auto vertices_dfs(G& g, adj_list::vertex_id_t seed, VVF&& vvf) { +[[nodiscard]] auto vertices_dfs(G& g, const adj_list::vertex_id_t& seed, VVF&& vvf) { return vertices_dfs_view, std::allocator>(g, seed, std::forward(vvf), std::allocator{}); } @@ -664,7 +664,7 @@ requires vertex_value_function> */ template requires(!vertex_value_function>) -[[nodiscard]] auto vertices_dfs(G& g, adj_list::vertex_id_t seed, Alloc alloc) { +[[nodiscard]] auto vertices_dfs(G& g, const adj_list::vertex_id_t& seed, Alloc alloc) { return vertices_dfs_view(g, seed, alloc); } @@ -705,7 +705,7 @@ requires(!vertex_value_function>) */ template requires vertex_value_function> -[[nodiscard]] auto vertices_dfs(G& g, adj_list::vertex_id_t seed, VVF&& vvf, Alloc alloc) { +[[nodiscard]] auto vertices_dfs(G& g, const adj_list::vertex_id_t& seed, VVF&& vvf, Alloc alloc) { return vertices_dfs_view, Alloc>(g, seed, std::forward(vvf), alloc); } @@ -881,7 +881,7 @@ class edges_dfs_view : public std::ranges::view_interf : g_(&g), state_(std::make_shared(g, seed_vertex, adj_list::num_vertices(g), alloc)) {} /// Construct from vertex ID (delegates to vertex descriptor constructor) - edges_dfs_view(G& g, vertex_id_type seed, Alloc alloc = {}) + edges_dfs_view(G& g, const vertex_id_type& seed, Alloc alloc = {}) : edges_dfs_view(g, *adj_list::find_vertex(g, seed), alloc) {} [[nodiscard]] iterator begin() { return iterator(g_, state_); } @@ -1053,7 +1053,7 @@ class edges_dfs_view : public std::ranges::view_interface(g, seed_vertex, adj_list::num_vertices(g), alloc)) {} /// Construct from vertex ID (delegates to vertex descriptor constructor) - edges_dfs_view(G& g, vertex_id_type seed, EVF evf, Alloc alloc = {}) + edges_dfs_view(G& g, const vertex_id_type& seed, EVF evf, Alloc alloc = {}) : edges_dfs_view(g, *adj_list::find_vertex(g, seed), std::move(evf), alloc) {} [[nodiscard]] iterator begin() { return iterator(g_, state_, &evf_); } @@ -1080,13 +1080,13 @@ class edges_dfs_view : public std::ranges::view_interface> -edges_dfs_view(G&, adj_list::vertex_id_t, Alloc) -> edges_dfs_view; +edges_dfs_view(G&, const adj_list::vertex_id_t&, Alloc) -> edges_dfs_view; template -edges_dfs_view(G&, adj_list::vertex_id_t) -> edges_dfs_view>; +edges_dfs_view(G&, const adj_list::vertex_id_t&) -> edges_dfs_view>; template > -edges_dfs_view(G&, adj_list::vertex_id_t, EVF, Alloc) -> edges_dfs_view; +edges_dfs_view(G&, const adj_list::vertex_id_t&, EVF, Alloc) -> edges_dfs_view; // Deduction guides for edges_dfs_view - vertex descriptor template > @@ -1118,7 +1118,7 @@ edges_dfs_view(G&, adj_list::vertex_t, EVF, Alloc) -> edges_dfs_view -[[nodiscard]] auto edges_dfs(G& g, adj_list::vertex_id_t seed) { +[[nodiscard]] auto edges_dfs(G& g, const adj_list::vertex_id_t& seed) { return edges_dfs_view>(g, seed, std::allocator{}); } @@ -1158,7 +1158,7 @@ template */ template requires edge_value_function> -[[nodiscard]] auto edges_dfs(G& g, adj_list::vertex_id_t seed, EVF&& evf) { +[[nodiscard]] auto edges_dfs(G& g, const adj_list::vertex_id_t& seed, EVF&& evf) { return edges_dfs_view, std::allocator>(g, seed, std::forward(evf), std::allocator{}); } @@ -1198,7 +1198,7 @@ requires edge_value_function> */ template requires(!edge_value_function>) -[[nodiscard]] auto edges_dfs(G& g, adj_list::vertex_id_t seed, Alloc alloc) { +[[nodiscard]] auto edges_dfs(G& g, const adj_list::vertex_id_t& seed, Alloc alloc) { return edges_dfs_view(g, seed, alloc); } @@ -1239,7 +1239,7 @@ requires(!edge_value_function>) */ template requires edge_value_function> -[[nodiscard]] auto edges_dfs(G& g, adj_list::vertex_id_t seed, EVF&& evf, Alloc alloc) { +[[nodiscard]] auto edges_dfs(G& g, const adj_list::vertex_id_t& seed, EVF&& evf, Alloc alloc) { return edges_dfs_view, Alloc>(g, seed, std::forward(evf), alloc); } @@ -1273,7 +1273,7 @@ requires edge_value_function> /// DFS vertex traversal with explicit Accessor, from vertex ID. template -[[nodiscard]] auto vertices_dfs(G& g, adj_list::vertex_id_t seed) { +[[nodiscard]] auto vertices_dfs(G& g, const adj_list::vertex_id_t& seed) { return vertices_dfs_view, Accessor>(g, seed, std::allocator{}); } @@ -1286,7 +1286,7 @@ template /// DFS vertex traversal with explicit Accessor and value function, from vertex ID. template requires vertex_value_function> -[[nodiscard]] auto vertices_dfs(G& g, adj_list::vertex_id_t seed, VVF&& vvf) { +[[nodiscard]] auto vertices_dfs(G& g, const adj_list::vertex_id_t& seed, VVF&& vvf) { return vertices_dfs_view, std::allocator, Accessor>(g, seed, std::forward(vvf), std::allocator{}); } @@ -1302,7 +1302,7 @@ requires vertex_value_function> /// DFS edge traversal with explicit Accessor, from vertex ID. template -[[nodiscard]] auto edges_dfs(G& g, adj_list::vertex_id_t seed) { +[[nodiscard]] auto edges_dfs(G& g, const adj_list::vertex_id_t& seed) { return edges_dfs_view, Accessor>(g, seed, std::allocator{}); } @@ -1315,7 +1315,7 @@ template /// DFS edge traversal with explicit Accessor and value function, from vertex ID. template requires edge_value_function> -[[nodiscard]] auto edges_dfs(G& g, adj_list::vertex_id_t seed, EVF&& evf) { +[[nodiscard]] auto edges_dfs(G& g, const adj_list::vertex_id_t& seed, EVF&& evf) { return edges_dfs_view, std::allocator, Accessor>(g, seed, std::forward(evf), std::allocator{}); } diff --git a/include/graph/views/incidence.hpp b/include/graph/views/incidence.hpp index 654fadf..d78191e 100644 --- a/include/graph/views/incidence.hpp +++ b/include/graph/views/incidence.hpp @@ -411,7 +411,7 @@ requires edge_value_function> * @post The graph is not modified. */ template -[[nodiscard]] constexpr auto incidence(G& g, adj_list::vertex_id_t uid) { +[[nodiscard]] constexpr auto incidence(G& g, const adj_list::vertex_id_t& uid) { auto u = *adj_list::find_vertex(g, uid); return incidence(g, u); } @@ -440,7 +440,7 @@ template */ template requires edge_value_function> -[[nodiscard]] constexpr auto incidence(G& g, adj_list::vertex_id_t uid, EVF&& evf) { +[[nodiscard]] constexpr auto incidence(G& g, const adj_list::vertex_id_t& uid, EVF&& evf) { auto u = *adj_list::find_vertex(g, uid); return incidence(g, u, std::forward(evf)); } @@ -704,7 +704,7 @@ basic_incidence_view(G&, adj_list::vertex_t, EVF) -> basic_incidence_view -[[nodiscard]] constexpr auto basic_incidence(G& g, adj_list::vertex_id_t uid) { +[[nodiscard]] constexpr auto basic_incidence(G& g, const adj_list::vertex_id_t& uid) { auto u = *adj_list::find_vertex(g, uid); return basic_incidence_view(g, u); } @@ -730,7 +730,7 @@ template */ template requires edge_value_function> -[[nodiscard]] constexpr auto basic_incidence(G& g, adj_list::vertex_id_t uid, EVF&& evf) { +[[nodiscard]] constexpr auto basic_incidence(G& g, const adj_list::vertex_id_t& uid, EVF&& evf) { auto u = *adj_list::find_vertex(g, uid); return basic_incidence_view>(g, u, std::forward(evf)); } @@ -754,7 +754,7 @@ requires edge_value_function> /// @brief Create an outgoing incidence view from vertex id. template -[[nodiscard]] constexpr auto out_incidence(G& g, adj_list::vertex_id_t uid) { +[[nodiscard]] constexpr auto out_incidence(G& g, const adj_list::vertex_id_t& uid) { auto u = *adj_list::find_vertex(g, uid); return out_incidence(g, u); } @@ -762,14 +762,14 @@ template /// @brief Create an outgoing incidence view with EVF from vertex id. template requires edge_value_function> -[[nodiscard]] constexpr auto out_incidence(G& g, adj_list::vertex_id_t uid, EVF&& evf) { +[[nodiscard]] constexpr auto out_incidence(G& g, const adj_list::vertex_id_t& uid, EVF&& evf) { auto u = *adj_list::find_vertex(g, uid); return out_incidence(g, u, std::forward(evf)); } /// @brief Create a basic outgoing incidence view (target id only). template -[[nodiscard]] constexpr auto basic_out_incidence(G& g, adj_list::vertex_id_t uid) { +[[nodiscard]] constexpr auto basic_out_incidence(G& g, const adj_list::vertex_id_t& uid) { auto u = *adj_list::find_vertex(g, uid); return basic_incidence_view(g, u); } @@ -777,7 +777,7 @@ template /// @brief Create a basic outgoing incidence view with EVF. template requires edge_value_function> -[[nodiscard]] constexpr auto basic_out_incidence(G& g, adj_list::vertex_id_t uid, EVF&& evf) { +[[nodiscard]] constexpr auto basic_out_incidence(G& g, const adj_list::vertex_id_t& uid, EVF&& evf) { auto u = *adj_list::find_vertex(g, uid); return basic_incidence_view, out_edge_accessor>(g, u, std::forward(evf)); } @@ -801,7 +801,7 @@ requires edge_value_function> /// @brief Create an incoming incidence view from vertex id. template -[[nodiscard]] constexpr auto in_incidence(G& g, adj_list::vertex_id_t uid) { +[[nodiscard]] constexpr auto in_incidence(G& g, const adj_list::vertex_id_t& uid) { auto u = *adj_list::find_vertex(g, uid); return in_incidence(g, u); } @@ -809,14 +809,14 @@ template /// @brief Create an incoming incidence view with EVF from vertex id. template requires edge_value_function> -[[nodiscard]] constexpr auto in_incidence(G& g, adj_list::vertex_id_t uid, EVF&& evf) { +[[nodiscard]] constexpr auto in_incidence(G& g, const adj_list::vertex_id_t& uid, EVF&& evf) { auto u = *adj_list::find_vertex(g, uid); return in_incidence(g, u, std::forward(evf)); } /// @brief Create a basic incoming incidence view (source id only). template -[[nodiscard]] constexpr auto basic_in_incidence(G& g, adj_list::vertex_id_t uid) { +[[nodiscard]] constexpr auto basic_in_incidence(G& g, const adj_list::vertex_id_t& uid) { auto u = *adj_list::find_vertex(g, uid); return basic_incidence_view(g, u); } @@ -824,7 +824,7 @@ template /// @brief Create a basic incoming incidence view with EVF. template requires edge_value_function> -[[nodiscard]] constexpr auto basic_in_incidence(G& g, adj_list::vertex_id_t uid, EVF&& evf) { +[[nodiscard]] constexpr auto basic_in_incidence(G& g, const adj_list::vertex_id_t& uid, EVF&& evf) { auto u = *adj_list::find_vertex(g, uid); return basic_incidence_view, in_edge_accessor>(g, u, std::forward(evf)); } diff --git a/include/graph/views/neighbors.hpp b/include/graph/views/neighbors.hpp index 1081c5f..520779c 100644 --- a/include/graph/views/neighbors.hpp +++ b/include/graph/views/neighbors.hpp @@ -398,7 +398,7 @@ template * @post The graph is not modified. */ template -[[nodiscard]] constexpr auto neighbors(G& g, adj_list::vertex_id_t uid) noexcept { +[[nodiscard]] constexpr auto neighbors(G& g, const adj_list::vertex_id_t& uid) noexcept { auto u = *adj_list::find_vertex(g, uid); return neighbors(g, u); } @@ -452,7 +452,7 @@ requires vertex_value_function> */ template requires vertex_value_function> -[[nodiscard]] constexpr auto neighbors(G& g, adj_list::vertex_id_t uid, VVF&& vvf) { +[[nodiscard]] constexpr auto neighbors(G& g, const adj_list::vertex_id_t& uid, VVF&& vvf) { auto u = *adj_list::find_vertex(g, uid); return neighbors(g, u, std::forward(vvf)); } @@ -722,7 +722,7 @@ basic_neighbors_view(G&, adj_list::vertex_t, VVF) -> basic_neighbors_view -[[nodiscard]] constexpr auto basic_neighbors(G& g, adj_list::vertex_id_t uid) { +[[nodiscard]] constexpr auto basic_neighbors(G& g, const adj_list::vertex_id_t& uid) { auto u = *adj_list::find_vertex(g, uid); return basic_neighbors_view(g, u); } @@ -748,7 +748,7 @@ template */ template requires vertex_value_function> -[[nodiscard]] constexpr auto basic_neighbors(G& g, adj_list::vertex_id_t uid, VVF&& vvf) { +[[nodiscard]] constexpr auto basic_neighbors(G& g, const adj_list::vertex_id_t& uid, VVF&& vvf) { auto u = *adj_list::find_vertex(g, uid); return basic_neighbors_view>(g, u, std::forward(vvf)); } @@ -772,7 +772,7 @@ requires vertex_value_function> /// @brief Create an outgoing neighbors view from vertex id. template -[[nodiscard]] constexpr auto out_neighbors(G& g, adj_list::vertex_id_t uid) { +[[nodiscard]] constexpr auto out_neighbors(G& g, const adj_list::vertex_id_t& uid) { auto u = *adj_list::find_vertex(g, uid); return out_neighbors(g, u); } @@ -780,14 +780,14 @@ template /// @brief Create an outgoing neighbors view with VVF from vertex id. template requires vertex_value_function> -[[nodiscard]] constexpr auto out_neighbors(G& g, adj_list::vertex_id_t uid, VVF&& vvf) { +[[nodiscard]] constexpr auto out_neighbors(G& g, const adj_list::vertex_id_t& uid, VVF&& vvf) { auto u = *adj_list::find_vertex(g, uid); return out_neighbors(g, u, std::forward(vvf)); } /// @brief Create a basic outgoing neighbors view (target id only). template -[[nodiscard]] constexpr auto basic_out_neighbors(G& g, adj_list::vertex_id_t uid) { +[[nodiscard]] constexpr auto basic_out_neighbors(G& g, const adj_list::vertex_id_t& uid) { auto u = *adj_list::find_vertex(g, uid); return basic_neighbors_view(g, u); } @@ -795,7 +795,7 @@ template /// @brief Create a basic outgoing neighbors view with VVF. template requires vertex_value_function> -[[nodiscard]] constexpr auto basic_out_neighbors(G& g, adj_list::vertex_id_t uid, VVF&& vvf) { +[[nodiscard]] constexpr auto basic_out_neighbors(G& g, const adj_list::vertex_id_t& uid, VVF&& vvf) { auto u = *adj_list::find_vertex(g, uid); return basic_neighbors_view, out_edge_accessor>(g, u, std::forward(vvf)); } @@ -819,7 +819,7 @@ requires vertex_value_function> /// @brief Create an incoming neighbors view from vertex id. template -[[nodiscard]] constexpr auto in_neighbors(G& g, adj_list::vertex_id_t uid) { +[[nodiscard]] constexpr auto in_neighbors(G& g, const adj_list::vertex_id_t& uid) { auto u = *adj_list::find_vertex(g, uid); return in_neighbors(g, u); } @@ -827,14 +827,14 @@ template /// @brief Create an incoming neighbors view with VVF from vertex id. template requires vertex_value_function> -[[nodiscard]] constexpr auto in_neighbors(G& g, adj_list::vertex_id_t uid, VVF&& vvf) { +[[nodiscard]] constexpr auto in_neighbors(G& g, const adj_list::vertex_id_t& uid, VVF&& vvf) { auto u = *adj_list::find_vertex(g, uid); return in_neighbors(g, u, std::forward(vvf)); } /// @brief Create a basic incoming neighbors view (source id only). template -[[nodiscard]] constexpr auto basic_in_neighbors(G& g, adj_list::vertex_id_t uid) { +[[nodiscard]] constexpr auto basic_in_neighbors(G& g, const adj_list::vertex_id_t& uid) { auto u = *adj_list::find_vertex(g, uid); return basic_neighbors_view(g, u); } @@ -842,7 +842,7 @@ template /// @brief Create a basic incoming neighbors view with VVF. template requires vertex_value_function> -[[nodiscard]] constexpr auto basic_in_neighbors(G& g, adj_list::vertex_id_t uid, VVF&& vvf) { +[[nodiscard]] constexpr auto basic_in_neighbors(G& g, const adj_list::vertex_id_t& uid, VVF&& vvf) { auto u = *adj_list::find_vertex(g, uid); return basic_neighbors_view, in_edge_accessor>(g, u, std::forward(vvf)); } diff --git a/include/graph/views/transpose.hpp b/include/graph/views/transpose.hpp index 1cfc55c..67f7031 100644 --- a/include/graph/views/transpose.hpp +++ b/include/graph/views/transpose.hpp @@ -93,10 +93,10 @@ class transpose_view { friend constexpr auto num_vertices(const transpose_view& tv) { return adj_list::num_vertices(*tv.g_); } - friend constexpr auto find_vertex(transpose_view& tv, adj_list::vertex_id_t uid) { + friend constexpr auto find_vertex(transpose_view& tv, const adj_list::vertex_id_t& uid) { return adj_list::find_vertex(*tv.g_, uid); } - friend constexpr auto find_vertex(const transpose_view& tv, adj_list::vertex_id_t uid) { + friend constexpr auto find_vertex(const transpose_view& tv, const adj_list::vertex_id_t& uid) { return adj_list::find_vertex(*tv.g_, uid); } diff --git a/tests/container/dynamic_graph/test_dynamic_graph_integration.cpp b/tests/container/dynamic_graph/test_dynamic_graph_integration.cpp index 79139a4..4bf267f 100644 --- a/tests/container/dynamic_graph/test_dynamic_graph_integration.cpp +++ b/tests/container/dynamic_graph/test_dynamic_graph_integration.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -95,6 +96,31 @@ using mol_string = dynamic_graph>; +//================================================================================================== +// Static assertions: verify vertex_id_t / raw_vertex_id_t / vertex_id_store_t resolution +//================================================================================================== + +// For vector-based graphs (integral VId): raw_vertex_id_t is NOT a reference +static_assert(!std::is_reference_v>, + "raw_vertex_id_t for vector-based graphs should be a value type (not a reference)"); +static_assert(sizeof(graph::adj_list::vertex_id_t) == sizeof(uint64_t) && + std::is_unsigned_v>, + "vertex_id_t for vector-based graphs should be a 64-bit unsigned integral type"); + +// For map-based graphs (string VId): raw_vertex_id_t IS a reference to the stable key +static_assert(std::is_reference_v>, + "raw_vertex_id_t for map-based graphs should be a reference type"); +static_assert(std::is_same_v, std::string>, + "vertex_id_t for map-based graphs should be std::string (value type)"); + +// vertex_id_store_t: value for integral, reference_wrapper for map-based +static_assert(sizeof(graph::vertex_id_store_t) == sizeof(uint64_t) && + std::is_unsigned_v>, + "vertex_id_store_t for vector-based graphs should be a 64-bit unsigned integral type"); +static_assert(std::is_same_v, + std::reference_wrapper>, + "vertex_id_store_t for map-based graphs should be reference_wrapper"); + //================================================================================================== // Helper: Count all edges //==================================================================================================