Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds many new graph algorithms, matchers, decompositions, examples, tests, CI jobs, and container utilities: unweighted/weighted Edmonds blossom, min-cost/perfect matching wrappers, Hungarian assignment, k-shortest paths, Karp min-mean-cycle, LCA engines, HLD/centroid decompositions, Tarjan bridges, Lengauer–Tarjan dominators, plus examples/tests and CMake/CI updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BlossomAPI as Blossom API
participant Matcher as Edmonds_Blossom_Matcher
participant BlossomOps as Blossom Contraction/Expansion
participant BFS as Alternating-Path BFS
Client->>BlossomAPI: call compute_maximum_cardinality_general_matching(graph, out)
BlossomAPI->>Matcher: init with graph
Matcher->>Matcher: build index & adjacency
loop find augmenting paths
Matcher->>BFS: search augmenting path (BFS)
BFS-->>Matcher: path / none
alt path found with blossom
Matcher->>BlossomOps: contract blossom (LCA & mark)
BlossomOps-->>Matcher: contracted graph
Matcher->>Matcher: augment_matching(path)
else path found without blossom
Matcher->>Matcher: augment_matching(path)
end
end
Matcher-->>BlossomAPI: return mate vector
BlossomAPI->>BlossomAPI: map mates to `Arc*` and return cardinality
sequenceDiagram
participant Client
participant HLDInit as HLD Initialization
participant Topology as Tree Topology Builder
participant Decompose as Heavy-Light Decompose
participant Segment as Segment Tree
Client->>HLDInit: construct HLD with graph, root, value accessor
HLDInit->>Topology: index nodes, validate tree
Topology->>Topology: DFS compute parent, depth, subtree sizes
Topology-->>Decompose: traversal order
Decompose->>Decompose: compute heavy child, chain heads, positions
Decompose->>Segment: build segment tree over HLD order
Segment-->>HLDInit: ready
Client->>HLDInit: path_query(u,v)
HLDInit->>Segment: query segments along path (O(log^2 n))
Segment-->>Client: aggregated result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80e57ad345
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
This PR significantly expands Aleph-w’s graph and tree algorithm offerings by adding general-graph matching/assignment algorithms and tree-query utilities, plus associated documentation, examples, and tests.
Changes:
- Add general graph matching suite: Edmonds blossom (cardinality), weighted blossom (maximum weight), and a minimum-cost matching wrapper built on weighted blossom.
- Add tree utilities and demos: LCA (binary lifting + Euler/RMQ) and tree decompositions (HLD + centroid), with new test coverage.
- Add bridge-finding (Tarjan low-link) alongside cut nodes / biconnected components, plus various doc/style updates.
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tpl_sort_utils.H | Minor formatting/indentation adjustments in sorting utilities. |
| tpl_cut_nodes.H | Expanded documentation and added Compute_Bridges/find_bridges implementation. |
| doxygen_groups.H | Doxygen group list updated to mention matching and LCA. |
| ahFunctional.H | Added in-place fill()/iota() algorithms and all_unique() helper. |
| ah-dry.H | Small grammar fix in documentation. |
| ah-concepts.H | Added AlephSequentialContainer / AlephSequentialIterableContainer concepts. |
| Tests/tree_decomposition_test.cc | New tests for HLD and centroid decomposition across graph backends. |
| Tests/lca_test.cc | New tests for LCA.H implementations (binary lifting and Euler+RMQ). |
| Tests/geom_algorithms_test_common.h | Refactor/format helper utilities and comments. |
| Tests/cut_nodes_test.cc | Added bridge-finding test suite. |
| Tests/ah_concepts_test.cc | Added concept static_asserts and tests for fill() / iota(). |
| Tests/CMakeLists.txt | Registered new tests (incl. LCA and matching-related tests). |
| README.es.md | Updated Spanish README with expanded algorithm listings and examples. |
| Min_Cost_Matching.H | New minimum-cost (and perfect) matching API on general graphs. |
| Hungarian.H | New Hungarian/Munkres assignment implementation and helpers. |
| Blossom_Weighted.H | New maximum-weight matching API and wrapper around mwmatching backend. |
| Blossom.H | New Edmonds blossom implementation for maximum cardinality matching. |
| Examples/blossom_example.cc | New example program for cardinality blossom + TikZ export. |
| Examples/weighted_blossom_example.cc | New example program for weighted blossom + TikZ export. |
| Examples/lca_example.cc | New example program demonstrating LCA engines and parity/benchmarking. |
| Examples/hungarian_example.cc | New example program for Hungarian assignment (min and max). |
| Examples/hld_example.cc | New example program demonstrating HLD use-cases. |
| Examples/heavy_light_decomposition_example.cc | New story-driven HLD demo using Tree_Decomposition.H. |
| Examples/centroid_decomposition_example.cc | New story-driven centroid decomposition demo. |
| Examples/cut_nodes_example.C | Updated example to include bridges demo and refreshed docs/CLI flags. |
| Examples/CMakeLists.txt | Registered new example executables. |
| CMakeLists.txt | Added new headers to build/install lists (matching, LCA/HLD, decompositions). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
ah-dry.H (5)
1701-1711:⚠️ Potential issue | 🔴 Critical
items_ptr()— two type errors that prevent compilation on instantiation.
- Line 1704:
C<Data> ret_valshould beC<std::pair<Key, Data*>>— the declared return type and the container type disagree.- Line 1708:
p.secondisData, notData*— it must be&p.second.🐛 Proposed fix
template <template <typename> class C = Aleph::DynList> C<std::pair<Key, Data*>> items_ptr() const { - C<Data> ret_val; + C<std::pair<Key, Data*>> ret_val; const_me()->for_each([&ret_val] (std::pair<Key, Data> & p) { - ret_val.append(std::pair<Key,Data*>(p.first, - p.second)); + ret_val.append(std::pair<Key,Data*>(p.first, + &p.second)); }); return ret_val; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ah-dry.H` around lines 1701 - 1711, In items_ptr(), the local container is declared with the wrong element type and the lambda appends a Data value instead of a pointer: change the local variable ret_val to type C<std::pair<Key, Data*>> (matching the function return type) and, inside the for_each lambda, append std::pair<Key, Data*>(p.first, &p.second) (take the address of p.second) so the stored second element is a Data*; keep the rest of items_ptr(), const_me()->for_each and the lambda structure unchanged.
766-779:⚠️ Potential issue | 🟠 Major
each(pos, slice, operation)— infinite loop whenslice == 0.When
sliceis0the innerforloop executes zero iterations, the outerwhile (true)never advances the iterator, andoperationis called on the same element forever. The analogoustake(i, j, step)at line 1449 already guards againststep == 0; the same guard is needed here.🐛 Proposed fix — guard against zero slice
template <class Operation> void each(size_t pos, const size_t slice, Operation & operation) const { + if (slice == 0) + return; auto it = const_me()->get_it(pos); - while (true) + while (it.has_curr()) { operation(it.get_curr()); for (size_t k = 0; k < slice; ++k) { it.next(); if (not it.has_curr()) return; } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ah-dry.H` around lines 766 - 779, The each(size_t pos, const size_t slice, Operation & operation) method can infinite-loop when slice == 0 because the inner for-loop never advances the iterator; add the same guard as in take(i, j, step) to check slice == 0 at the start of each (or throw/assert) and handle it (e.g. return immediately or raise an error). Locate the each function (uses const_me()->get_it(pos), it.next(), it.has_curr()) and add a guard at the top to prevent slice == 0 before entering the while(true) loop.
788-796:⚠️ Potential issue | 🟡 Minor
mutable_for_each— missing Doxygen on primary overload.The primary
mutable_for_each(Operation&)overload has no documentation at all. As per coding guidelines, all public member functions must have@brief,@param,@return, and@throwsDoxygen tags.📝 Proposed doc addition
+ /** Traverse the container and perform a mutable operation on each element. + * + * Unlike `for_each`, the operation receives a non-const `T&` reference, + * allowing in-place mutation. Use with care on ordered containers (trees, + * heaps, hash tables) as mutation may corrupt internal invariants. + * + * `@param`[in] operation callable with signature `void(T&)` + * `@throws` anything that `operation` throws + */ template <class Operation> void mutable_for_each(Operation & operation)As per coding guidelines: "All public member functions must have
@brief,@param,@return, and@throwsDoxygen tags."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ah-dry.H` around lines 788 - 796, Add a Doxygen comment block for the primary template method mutable_for_each(Operation&), documenting its purpose and behavior; include `@brief` describing that it applies a mutable operation to each T element via me()->traverse, an `@param` for the Operation& (what it should accept and that it may modify items), an `@return` (e.g., void) and an `@throws` describing any exceptions that may propagate from the operation or traverse calls; place the comment immediately above the mutable_for_each template declaration so it covers the primary overload.
1302-1315:⚠️ Potential issue | 🔴 Critical
partition(size_t n)— spurioustemplate <class Operation>makes the overload uncallable.
Operationappears in neither the parameter list nor the function body, so it is non-deducible and has no default value. If any template argument remains neither deduced nor explicitly specified, compilation fails. Any call of the formcontainer.partition(n)will fail deduction; the function is effectively unreachable without an uglycontainer.template partition<void>(n).🐛 Proposed fix — remove the spurious template parameter
- template <class Operation> std::pair<Aleph::DynList<T>, Aleph::DynList<T>> partition(size_t n) const🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ah-dry.H` around lines 1302 - 1315, The partition member function mistakenly declares an unused template parameter Operation, making the overload non-deducible; remove the spurious "template <class Operation>" so the signature becomes a non-template member function partition(size_t n) const, leaving the body (use of const_me(), ret_val, Aleph::DynList<T>, i, and the lambda) unchanged; ensure no other callers rely on an explicit template parameter for partition and rebuild to confirm the function is callable as container.partition(n).
994-994:⚠️ Potential issue | 🟡 MinorIncorrect formula in
foldldoc comment.The pseudo-code at line 994 reads:
acc = op(acc, op(acc, item);This has a nested
opcall and an unbalanced parenthesis. The actual implementation (line 1041) correctly applies a single call. The doc should read:acc = op(acc, item);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ah-dry.H` at line 994, The doc comment for foldl contains an incorrect pseudo-code line "acc = op(acc, op(acc, item);" — update the comment in the foldl documentation (referencing the foldl function) to the correct expression "acc = op(acc, item);" by removing the nested op call and fixing the unmatched parenthesis so the doc matches the implementation.Examples/cut_nodes_example.C (1)
373-381:⚠️ Potential issue | 🟡 MinorOff-by-one:
paint_subgraphs()returns the next unused color.Using that value as the component count and iterating
<= num_colorsoverstates by one. Usenum_colors - 1for the count and loop while< next_color.🛠️ Suggested fix
- long num_colors = compute.paint_subgraphs(); + long next_color = compute.paint_subgraphs(); + const long num_colors = next_color - 1; ... - cout << "Number of components: " << num_colors << endl; + cout << "Number of components: " << num_colors << endl; ... - for (long color = 1; color <= num_colors; ++color) + for (long color = 1; color < next_color; ++color)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/cut_nodes_example.C` around lines 373 - 381, paint_subgraphs() returns the next unused color, so using its return value as the component count is off-by-one; update the code that reads long num_colors = compute.paint_subgraphs() to treat num_colors as next_color (i.e., use num_colors - 1 when displaying "Number of components") and change the loop to iterate from color = 1 while color < num_colors (or equivalently color <= num_colors - 1) when printing nodes per component; reference compute.paint_subgraphs(), the num_colors variable, and the for loop that currently uses <= num_colors.Tests/geom_algorithms_test_common.h (1)
34-36:⚠️ Potential issue | 🟡 MinorGuard
circumcenter_ofagainst collinear triangles.
denis twice the signed area; for collinear points it becomes 0 and triggers division by zero. Please add a guard (prefer Aleph error macros) before the division.🔧 Suggested fix
`#include` <gtest/gtest.h> `#include` <geom_algorithms.H> +#include <ah-errors.H> `#include` "geometry_visual_golden.h" @@ const Geom_Number d = ax * (by - cy) + bx * (cy - ay) + cx * (ay - by); const Geom_Number den = d + d; + ah_domain_error_if(den == 0) + << "circumcenter_of: collinear points"; return {Also applies to: 155-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/geom_algorithms_test_common.h` around lines 34 - 36, The function circumcenter_of computes a denominator den (twice the signed area) and currently divides by it unguarded; add a check in circumcenter_of immediately before the division that tests if fabs(den) (or den == 0) is below a small epsilon and, if so, raise an Aleph error macro (e.g. ALEPH_ERROR or the project’s standard error macro) with a clear message like "collinear triangle in circumcenter_of" to prevent the division-by-zero; apply the same guard to the other occurrence of the same code block (lines ~155-174) so both divisions are protected.
🟡 Minor comments (12)
Min_Cost_Matching.H-241-259 (1)
241-259:⚠️ Potential issue | 🟡 MinorAdd required Doxygen tags to public aliases/wrappers.
The
blossom_minimum_cost_matching()alias and related wrappers omit@param/@return/@throwstags, which violates the public‑API documentation rules. As per coding guidelines: "All public member functions must have@brief,@param,@return, and@throwsDoxygen tags" and "Doxygen must generate no warnings for changed files."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Min_Cost_Matching.H` around lines 241 - 259, Add missing Doxygen tags to the public alias blossom_minimum_cost_matching so it documents parameters, return value, and possible exceptions: add `@param` entries for g (graph reference), matching (DynDlist of Arc* to receive the matching), cost (cost functor), sa (show-arc functor), and max_cardinality (bool), an `@return` describing the Min_Cost_Matching_Result<long long> returned, and an `@throws` describing any exceptions that may propagate from compute_minimum_cost_general_matching (e.g., allocation or algorithm errors); keep the existing `@brief` and mention that this function forwards to compute_minimum_cost_general_matching in the description.Hungarian.H-361-431 (1)
361-431:⚠️ Potential issue | 🟡 MinorAdd required
@throwstags to public accessors.Public methods like
get_total_cost(),get_assignments(),get_row_assignments(),dimension(),rows(), andcols()are missing@throwstags. Please add them (even if “none”) to satisfy Doxygen requirements. As per coding guidelines: "All public member functions must have@brief,@param,@return, and@throwsDoxygen tags" and "Doxygen must generate no warnings for changed files."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Hungarian.H` around lines 361 - 431, The Doxygen warnings come from missing `@throws` tags on the public accessors; add a `@throws` section (use "none" if they cannot throw) to each affected function comment block—specifically update get_total_cost(), get_assignments(), get_row_assignments(), get_col_assignments(), dimension(), rows(), and cols() to include an `@throws` tag (e.g., "@throws none") so all public member functions have `@brief`, `@param`, `@return`, and `@throws` tags and Doxygen emits no warnings.Tree_Decomposition.H-571-719 (1)
571-719:⚠️ Potential issue | 🟡 MinorAdd full Doxygen tags for public accessors.
Public methods like
size(),root(),node_of(),parent_id(),depth_of_id(), anddistance_id()are missing required@param/@return/@throwstags. Please update these to satisfy documentation rules. As per coding guidelines: "All public member functions must have@brief,@param,@return, and@throwsDoxygen tags" and "Doxygen must generate no warnings for changed files."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tree_Decomposition.H` around lines 571 - 719, Add full Doxygen comments for all public accessors (e.g., size, is_empty, root, root_id, node_of, id_of, parent_id, parent_of, depth_of_id, depth_of, subtree_size_of_id, subtree_size_of, heavy_child_id, heavy_child, head_id, head_of, position_of_id, position_of, subtree_range_id, subtree_range, is_ancestor_id, is_ancestor, lca_id, lca, distance_id, distance) including `@brief`, `@param` for each input parameter, `@return` describing the result, and `@throws` where the method can validate or throw (e.g., calls to topology_.validate_id or ensure_not_empty); ensure descriptions mention special return values like NONE mapping to nullptr where applicable; follow existing Doxygen style in the file and run the Doxygen generator to confirm zero warnings.HLD.H-600-623 (1)
600-623:⚠️ Potential issue | 🟡 MinorComplete Doxygen tags for public accessors.
Accessors like
size(),is_empty(),root(),root_id(),node_of(), andid_of()lack the required@param/@return/@throwstags, which will trigger Doxygen warnings for a public header. Please update these to include the full tag set. As per coding guidelines: "All public member functions must have@brief,@param,@return, and@throwsDoxygen tags" and "Doxygen must generate no warnings for changed files."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@HLD.H` around lines 600 - 623, Add full Doxygen tags to each public accessor: for size(), is_empty(), root(), root_id(), node_of(const size_t), and id_of(const Node*). Keep the existing `@brief` text and add an `@return` describing the returned value for every function; add `@param` for node_of (id) and id_of (node) describing the parameter; and add an `@throws` tag for each (use "None" or "No exceptions thrown" since all are noexcept). Ensure tags are placed in the same comment block above each method (e.g., above size(), node_of(), id_of(), root(), root_id(), is_empty()) so Doxygen warnings are eliminated.blossom_weighted_mwmatching.H-1908-1910 (1)
1908-1910:⚠️ Potential issue | 🟡 MinorReplace
!withnotper style rules.This header uses the
!operator instead of the keywordnot. The file contains 2 instances that need correction:
- Line 1908:
while (! stack.is_empty())- Line 2200:
ah_invalid_argument_if(! std::isfinite(edge.weight))Per coding guidelines: "Use and, or, not for logical operations (instead of &&, ||, !)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blossom_weighted_mwmatching.H` around lines 1908 - 1910, Replace the use of the C-style negation operator with the keyword form per style rules: change occurrences that negate boolean expressions to use "not" instead of "!" — specifically update the loop condition in the method containing "while (! stack.is_empty())" to "while (not stack.is_empty())" and update the check inside ah_invalid_argument_if from "ah_invalid_argument_if(! std::isfinite(edge.weight))" to "ah_invalid_argument_if(not std::isfinite(edge.weight))"; ensure spacing and parentheses remain correct around the expressions.Examples/hungarian_example.cc-58-63 (1)
58-63:⚠️ Potential issue | 🟡 MinorBug: prints total cost instead of individual assignment cost.
ha.get_total_cost()returns the overall optimal cost, not the cost of a single assignment. Each line in this loop will print the same value (the total), which is misleading. The "Detailed" section below (lines 72–76) correctly usescosts[r][c]for per-assignment costs. This first loop should do the same, or simply omit the cost column here since the detailed section follows immediately.Proposed fix
cout << "Assignments:" << endl; for (auto [r, c] : ha.get_assignments()) cout << " Worker " << r << " -> Task " << c - << " (cost " << ha.get_total_cost() << ")" << endl; + << endl; cout << endl;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/hungarian_example.cc` around lines 58 - 63, The loop printing assignments uses ha.get_total_cost() for each row which is incorrect; change the per-assignment cost to the matrix value costs[r][c] (the same approach used later in the "Detailed" section) so the cout inside the for (auto [r, c] : ha.get_assignments()) prints costs[r][c] instead of ha.get_total_cost(); ensure the variable costs is in scope where get_assignments() is iterated (or omit the per-assignment cost column if you prefer).README.md-921-926 (1)
921-926:⚠️ Potential issue | 🟡 MinorApply the flagged style nits (readability).
- The “Problem patterns solved” bullets start with “Dynamic …” multiple times (LanguageTool repetition warning).
- “full service area” should be “full-service area” if used as a compound adjective.
Proposed tweaks
- - **Dynamic path aggregates**: `sum/min/max/xor` on `path(u, v)` with node point updates. - - **Dynamic subtree aggregates**: full subtree query `subtree(u)` with node point updates. - - **Dynamic nearest active node** (unweighted distance in edges): mark nodes as active and query `min dist(u, active)`. - - **Dynamic farthest/threshold-style distance queries** by scanning centroid ancestor chains. + - **Online path aggregates**: `sum/min/max/xor` on `path(u, v)` with node point updates. + - **Online subtree aggregates**: full subtree query `subtree(u)` with node point updates. + - **Nearest active node queries** (unweighted distance in edges): mark nodes as active and query `min dist(u, active)`. + - **Farthest/threshold-style distance queries** by scanning centroid ancestor chains. @@ -Recipe 2: Subtree aggregate (full service area) +Recipe 2: Subtree aggregate (full-service area)Also applies to: 970-986
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 921 - 926, Revise the "Problem patterns solved:" bullet list (the header and bullets containing "Dynamic path aggregates", "Dynamic subtree aggregates", "Dynamic nearest active node", and the centroid chain note) to remove repeated sentence starts and improve readability—e.g., combine/variate phrasing so not every bullet begins with "Dynamic", reword "full subtree query `subtree(u)`" to use "full-service area" only if appropriate but otherwise hyphenate "full-service area" when used as a compound adjective, and apply the same style edits to the other occurrence of this list (the block around lines 970-986). Ensure you keep the technical names and inline code examples (`path(u, v)`, `subtree(u)`) unchanged while only adjusting surrounding wording for clarity and hyphenation.README.es.md-371-393 (1)
371-393:⚠️ Potential issue | 🟡 MinorRemove unnecessary escaping in prose and apply the flagged spacing fixes.
There are occurrences of
\"...\"in normal markdown text (not code). Markdown doesn’t require escaping quotes there, and LanguageTool flagged spacing around those sequences.Proposed fix (representative lines)
- Ejemplo: \"suma de riesgo entre sucursal A y B\" muchas veces. + Ejemplo: "suma de riesgo entre sucursal A y B" muchas veces. @@ - Ejemplo: \"costo total de una región operativa\" donde cambian nodos individuales. + Ejemplo: "costo total de una región operativa" donde cambian nodos individuales.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.es.md` around lines 371 - 393, Remove unnecessary backslash-escaped double quotes and correct spacing in the prose sections (not code) of the README: replace occurrences like \"suma de riesgo entre sucursal A y B\" with straight inline quotes "suma de riesgo entre sucursal A y B", and similarly for other quoted phrases; also fix any extra or missing spaces around those quotes flagged by LanguageTool. Ensure you only change plain markdown paragraphs (references to Tree_Decomposition.H, Gen_HLD_Path_Query, Gen_Centroid_Decomposition, LCA.H, tpl_mo_on_trees.H) and do not alter code blocks or inline code identifiers.Tests/blossom_test.cc-399-432 (1)
399-432:⚠️ Potential issue | 🟡 MinorTighten the bipartite cross-check assertion.
In
AgreesWithHopcroftKarpOnBipartiteGraphs, you compareblossom_sizeagainsthk_matching.size(), but don’t explicitly assertblossom_matching.size() == blossom_size(helpful if the API ever regresses and returns a mismatched cardinality vs list size).Proposed fix
const size_t blossom_size = blossom_maximum_cardinality_matching<Graph>(g, blossom_matching); hopcroft_karp_matching<Graph>(g, hk_matching); + EXPECT_EQ(blossom_matching.size(), blossom_size) + << "left=" << left << " right=" << right << " sample=" << sample; EXPECT_EQ(blossom_size, hk_matching.size()) << "left=" << left << " right=" << right << " sample=" << sample;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/blossom_test.cc` around lines 399 - 432, Add an explicit assertion that the returned cardinality blossom_size matches the actual list size of blossom_matching: after calling blossom_maximum_cardinality_matching<Graph>(g, blossom_matching) add EXPECT_EQ(blossom_size, blossom_matching.size()) (include the same contextual failure message with left/right/sample as used for the hk check) so we verify the function’s return value and the list length are consistent; keep the existing verify_matching checks and variable names (blossom_size, blossom_matching, hk_matching, blossom_maximum_cardinality_matching, hopcroft_karp_matching).Examples/weighted_blossom_example.cc-100-114 (1)
100-114:⚠️ Potential issue | 🟡 MinorGuard
Scenario.edgesindices inbuild_graph().
build_graph()assumes every(u, v)ins.edgesis in-range and will indexnodes[u]/nodes[v]directly (Line 110-112). This is fine for the current hardcoded scenarios, but it becomes a footgun when someone edits/extends scenarios.Proposed fix
for (const auto & [u, v, w] : s.edges) - g.insert_arc(nodes[u], nodes[v], w); + { + if (u >= s.nodes.size() or v >= s.nodes.size() or u == v) + continue; + g.insert_arc(nodes[u], nodes[v], w); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/weighted_blossom_example.cc` around lines 100 - 114, In build_graph(), validate each edge's endpoint indices before indexing nodes: check that u and v are within [0, nodes.size()) and only call g.insert_arc(nodes[u], nodes[v], w) when both are valid; for invalid indices either skip the edge (and optionally log a warning) or throw a clear exception; update the loop that processes s.edges (referencing Scenario::edges and the use of nodes[u]/nodes[v] and GT::Node pointers) to perform this guard.README.md-2088-2108 (1)
2088-2108:⚠️ Potential issue | 🟡 MinorFix undefined
nodes[2]in the bridges example snippet.The snippet uses
cb.find_bridges(nodes[2])(Line 2107), butnodesisn’t defined in the example, so readers can’t paste/compile it as-is.One minimal doc-only fix
- auto b2 = cb.find_bridges(nodes[2]); // start from a specific node + // Optional: start from a specific node pointer you already have in hand. + // (Define `start` when you build the graph.) + auto b2 = cb.find_bridges(start);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 2088 - 2108, The example calls cb.find_bridges(nodes[2]) but never defines nodes; fix by introducing a nodes collection from the graph before that call (e.g. obtain the graph's node list via g.get_nodes() or equivalent) and then call Compute_Bridges<Graph>::find_bridges with that node (nodes[2]); ensure the README snippet includes the nodes definition so cb.find_bridges(nodes[2]) compiles.README.es.md-85-151 (1)
85-151:⚠️ Potential issue | 🟡 MinorFix markdownlint MD040: add languages to fenced code blocks.
markdownlint reports missing language specifiers for fenced blocks (e.g., the ASCII tables). Use
textfor diagrams.Proposed fix
-``` +```text ┌────────────────────────────────────────────────────────────────────────────┐ │ RESUMEN DE ESTRUCTURAS DE DATOS │ ... └────────────────────────────────────────────────────────────────────────────┘@@
-+text
┌────────────────────────────────────────────────────────────────────────────┐
│ RESUMEN DE ALGORITMOS │
...
└────────────────────────────────────────────────────────────────────────────┘🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.es.md` around lines 85 - 151, The fenced ASCII-art code blocks for "RESUMEN DE ESTRUCTURAS DE DATOS" and "RESUMEN DE ALGORITMOS" are missing language specifiers; update each opening triple-backtick (```) for those two ASCII diagrams (the blocks that start with the box drawing ┌ and headings "RESUMEN DE ESTRUCTURAS DE DATOS" and "RESUMEN DE ALGORITMOS") to include the language tag "text" (i.e., change ``` to ```text) so markdownlint MD040 is satisfied.
🧹 Nitpick comments (4)
Examples/heavy_light_decomposition_example.cc (1)
143-210: Preferstd::formatoverprintfin C++20 examples.The example uses
std::printfthroughout; the project style favorsstd::formatfor type‑safe formatting. Consider switching tostd::format+std::cout(apply similarly to all print calls).As per coding guidelines: "Use std::format for type-safe string formatting (prefer over printf/streams)."🔧 Example pattern
+# include <format> @@ - std::printf(" id=%zu %-8s pos=%zu\n", i, labels[i], hld.position_of_id(i)); + std::cout << std::format(" id={} {:<8} pos={}\n", i, labels[i], hld.position_of_id(i));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/heavy_light_decomposition_example.cc` around lines 143 - 210, Replace all uses of std::printf in this example with C++20 type-safe formatting using std::format combined with std::cout (or std::puts if you want a newline-only). Specifically, update the prints around hld.size()/labels/hld.position_of_id, the "Path maintenance cost queries" loop that prints q.name/ans/brute, the subtree totals loop that prints labels[id]/ans, the budget change messages, the "North -> S-A2 after update" print that follows grid.update_node_id/grid.set_node_id, and the hld.for_each_path_segment print; include <format> and construct formatted strings via std::format(...) then output with std::cout << formatted << '\n' to replace each std::printf call. Ensure types/format specifiers are adjusted to std::format syntax (use {} placeholders) and keep existing variables like id_sa2/id_west, na/sa2, grid.query_path_id, brute_path_sum unchanged.Tests/hld_test.cc (1)
66-100: Consider extracting shared test helpers to reduce duplication.
build_graph_with_unit_arcs,Positive_Arc_Filter, andmake_random_tree_edgesare duplicated acrosslca_test.cc,hld_test.cc, and (per the snippets)tree_decomposition_test.cc. Extracting them into a shared test utility header would reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/hld_test.cc` around lines 66 - 100, The duplicated test helpers build_graph_with_unit_arcs, Positive_Arc_Filter, and make_random_tree_edges appear in multiple test files; extract these into a single shared test utility header (e.g., tests/utilities/test_graph_helpers.hpp) and replace the local copies by including that header in lca_test.cc, hld_test.cc, and tree_decomposition_test.cc. Move the template function build_graph_with_unit_arcs, the struct Positive_Arc_Filter, and the make_random_tree_edges implementation into the new header within the Aleph (or anonymous) test namespace, ensure all required headers/types are included or forward-declared there, and update the test files to remove the duplicates and add a single `#include` for the new header. Ensure symbol names (build_graph_with_unit_arcs, Positive_Arc_Filter, make_random_tree_edges) and their signatures remain unchanged so existing tests compile without other changes.Examples/weighted_blossom_example.cc (1)
55-64: Consider Aleph containers (and avoidusing namespace std;) for guideline compliance.This example leans heavily on
std::vector/std::set/std::tupleandusing namespace std;. If examples are meant to model “Aleph-first” style, consider switching to Aleph containers (or documenting why STL is preferred here, e.g., iostream/TikZ convenience).As per coding guidelines: “MUST use Aleph containers when an equivalent exists. Standard library containers are ONLY permitted when no Aleph equivalent is available.”
Also applies to: 83-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/weighted_blossom_example.cc` around lines 55 - 64, The example currently uses std::vector, std::set, std::tuple (and a global "using namespace std;"); replace those STL container usages with the corresponding Aleph container types and headers wherever an equivalent exists and remove the "using namespace std;" directive; update all occurrences of std::vector, std::set, std::tuple (and any unqualified container names that relied on the using directive) to the Aleph equivalents (and include the appropriate Aleph headers) so the file conforms to the "MUST use Aleph containers" guideline while keeping STL only for types with no Aleph counterpart.Tests/blossom_test.cc (1)
48-56: Use C++20std::countr_zerofrom<bit>instead of__builtin_ctzll.
__builtin_ctzllat lines 174 and 182 is a GCC/Clang builtin. Since the repo targets C++20,std::countr_zerois the portable standard library equivalent and aligns with the C++20 coding guidelines. Both calls are safe here (guaranteed non-zero preconditions via the function's early exit and loop condition), so the replacement is semantically correct.Proposed fix
# include <algorithm> +# include <bit> # include <cstdint> @@ - const size_t i = static_cast<size_t>(__builtin_ctzll(mask)); + const size_t i = static_cast<size_t>(std::countr_zero(mask)); @@ - const size_t j = static_cast<size_t>(__builtin_ctzll(options)); + const size_t j = static_cast<size_t>(std::countr_zero(options));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/blossom_test.cc` around lines 48 - 56, Replace the GCC/Clang builtin uses of __builtin_ctzll with the C++20 standard std::countr_zero and add `#include` <bit>; specifically, update the two call sites that currently call __builtin_ctzll(...) to std::countr_zero(...) (ensuring the argument type is an unsigned integer type compatible with std::countr_zero, e.g., static_cast<unsigned long long>(... ) if necessary) and add the <bit> include at the top of Tests/blossom_test.cc so the code uses the portable C++20 API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ahFunctional.H`:
- Around line 2442-2462: Add Doxygen `@overload` documentation for the newly added
templated overloads of all_unique (the rvalue-ref/Default-Equal overload:
template <AlephSequentialIterableContainer Container, class Equal =
std::equal_to<typename Container::Item_Type>> inline bool all_unique(const
Container & container, Equal && eq = Equal())) by adding a Doxygen block that
documents parameters, return value and marks it as `@overload` of the primary
all_unique(const Container&, Equal&), and implement a unit test that calls the
new overload with a non-trivial equality predicate (e.g., a case-insensitive
comparator for strings or a floating-point comparator with tolerance) to ensure
the overload is exercised and behavior verified; update test names to reference
all_unique overload and assert both true/false scenarios.
In `@blossom_weighted_mwmatching.H`:
- Around line 2175-2214: The safe minimum weight initialization uses
numeric_limits::min(), which for floating types is the smallest positive value
and will misflag negative weights; change the min_safe_weight calculation in the
function that declares min_safe_weight and max_safe_weight so that for
floating-point WeightType you use std::numeric_limits<WeightType>::lowest()/4
(or otherwise select lowest() when !std::numeric_limits<WeightType>::is_integer)
while keeping max_safe_weight = std::numeric_limits<WeightType>::max()/4; update
the min_safe_weight binding (and any related comments) so the later check
ah_invalid_argument_if(min_weight < min_safe_weight ...) behaves correctly for
floats.
In `@Examples/centroid_decomposition_example.cc`:
- Around line 45-51: Add a direct include for <algorithm> to this translation
unit so uses of std::min in Examples/centroid_decomposition_example.cc resolve
reliably (currently relying on transitive includes). Edit the top-of-file
includes (near the existing `#include` <Tree_Decomposition.H> and `#include`
<tpl_graph.H>) to add `#include` <algorithm>, ensuring the calls to std::min (used
around the centroid decomposition logic / main or related functions) compile on
all toolchains.
- Around line 93-110: The example assumes centroid/HLD ids equal the “Village-X”
indices; instead, after constructing Centroid_Decomposition<G> cd and
Heavy_Light_Decomposition<G> hld, build a mapping using cd.id_of(node) (e.g.,
create label_by_id and node_by_id arrays keyed by cd.id_of(node)) and then
replace direct uses of label[c], active(u) and literal id calls like
for_each_centroid_ancestor_id(10,...) with lookups via
label_by_id[cd.id_of(node)] or using node_by_id to pass correct node ids to hld
and for_each_centroid_ancestor_id so all printing, activation and oracle
distance assertions use the centroid/HLD id mapping rather than assuming 0..n
order.
In `@Examples/weighted_blossom_example.cc`:
- Around line 178-223: The TikZ output in write_tikz() currently emits edge
draws and edge labels (using node names like (nX) and $(nX)!0.5!(nY)$ from
s.edges and matched_set) before emitting the vertex node definitions from
s.nodes, which causes unknown-node errors; fix by writing the loop that emits
the "\\node[vertex] (n...)" definitions before the loop that emits edge "\\draw"
and wlabel nodes, and update the "edge" style in the \\tikzset (used in the
existing write_tikz()) to include shortening options (e.g., shorten <= and
shorten >=) so edges stop at vertex borders rather than drawing through nodes.
In `@Hungarian.H`:
- Around line 92-100: Add a compile-time guard that prevents using unsigned
types for Cost_Type by including <type_traits> and adding a static_assert near
the template/class declaration that defines Cost_Type (e.g., the
template<typename Cost_Type> for the Hungarian implementation) asserting
std::is_signed<std::remove_cv_t<Cost_Type>>::value ||
std::is_floating_point<std::remove_cv_t<Cost_Type>>::value, with a clear error
message like "Cost_Type must be signed or floating-point to avoid underflow when
negating costs"; apply the same static_assert at the other template declaration
referenced around lines 169-176.
- Around line 487-496: The negation can overflow for signed integral Cost_Type
(negating numeric_limits<Cost_Type>::min()), so update hungarian_max_assignment
to detect when Cost_Type is a signed integer and, in that case, promote values
to a wider signed integer type before negation: compute a Promoted =
std::make_signed_t<std::common_type_t<Cost_Type, long long>> (or similar),
allocate DynMatrix<Promoted> negated, fill with
-static_cast<Promoted>(cost.read(i,j)), call hungarian_assignment on that
matrix, then convert/negate result.total_cost back to Cost_Type (or the
caller-expected type) safely; if promotion isn't feasible, alternatively detect
presence of numeric_limits<Cost_Type>::min() and apply a uniform offset to all
entries prior to negation and adjust result.total_cost accordingly. Ensure
changes reference negated, Cost_Type, hungarian_assignment, and
result.total_cost.
In `@Min_Cost_Matching.H`:
- Around line 193-202: compute_minimum_cost_general_matching currently assumes
the graph is undirected but does not validate that, leading to undefined
behavior; add the same undirected-graph guard used by the perfect-matching
variant at the start of compute_minimum_cost_general_matching (check GT's
undirected/directed query method—e.g.
is_directed()/directed()/is_undirected()—and if the graph is directed, return an
error/throw std::invalid_argument or return an appropriate
Min_Cost_Matching_Result<long long> error state), so the blossom backend is
never invoked on a directed GT and behavior matches the
compute_minimum_cost_perfect_matching contract.
- Around line 119-137: to_ll_checked currently casts LLONG_MAX/MIN into T which
can narrow incorrectly; instead determine if T can exceed long long using
std::numeric_limits<T>::digits and only perform bounds checks when necessary,
and when checking convert value to a wider type (e.g. __int128) to compare
against LLONG_MAX/LLONG_MIN to avoid narrowing. Update the template function to:
use constexpr logic on std::numeric_limits<T>::digits and std::is_signed_v<T> to
decide whether an upper/lower check is required, and when required compare
static_cast<__int128>(value) to static_cast<__int128>(std::numeric_limits<long
long>::max()/min()) and trigger ah_overflow_error_if with the same message;
finally return static_cast<long long>(value) as before.
In `@Tests/cut_nodes_test.cc`:
- Around line 939-946: The new bridge tests currently use std::set and
std::vector which violates the Aleph-container rule; replace the std::set in the
helper bridge_set with DynSetTree<Graph::Arc*> and use Aleph list/array types
(DynList<Graph::Arc*> or DynArray<Graph::Arc*>) for the arc collections
referenced in the tests (including the other occurrences around the second block
noted at 1162-1164). Update the bridge_set signature and implementation to build
and return a DynSetTree<Graph::Arc*> by iterating the incoming
DynList<Graph::Arc*> (using the existing Iterator usage pattern), and replace
any std::vector usages in the tests with DynList or DynArray to preserve
semantics.
In `@Tests/min_cost_matching_test.cc`:
- Around line 46-58: Replace compiler-specific __builtin_ctzll calls with the
C++20 std::countr_zero for portability: add `#include` <bit> if not present and
change __builtin_ctzll(mask) and __builtin_ctzll(options) to
static_cast<int>(std::countr_zero(mask)) and
static_cast<int>(std::countr_zero(options)) (or equivalent casts used elsewhere)
in the DP oracle code (the mask/options usages), and make the identical
replacements in the other test file where __builtin_ctzll is used
(blossom_test.cc).
In `@tpl_cut_nodes.H`:
- Around line 716-749: Move the empty-graph check before validating start (so
find_bridges() returns immediately on gptr->get_num_nodes()==0 without error)
and change the DFS invocation to cover all components: after initializing
NODE_COUNTER/ NODE_BITS and resetting arcs, run __dfs(start, nullptr, curr_df,
bridges) for the provided start but then iterate gptr->for_each_node and for any
node with NODE_COUNTER==0 invoke __dfs(node, nullptr, curr_df, bridges) to
explore disconnected components; keep clearing NODE_COOKIE afterward. Use the
existing symbols find_bridges, __dfs, NODE_COUNTER, NODE_BITS, NODE_COOKIE, and
gptr->for_each_node to locate and implement the changes.
---
Outside diff comments:
In `@ah-dry.H`:
- Around line 1701-1711: In items_ptr(), the local container is declared with
the wrong element type and the lambda appends a Data value instead of a pointer:
change the local variable ret_val to type C<std::pair<Key, Data*>> (matching the
function return type) and, inside the for_each lambda, append std::pair<Key,
Data*>(p.first, &p.second) (take the address of p.second) so the stored second
element is a Data*; keep the rest of items_ptr(), const_me()->for_each and the
lambda structure unchanged.
- Around line 766-779: The each(size_t pos, const size_t slice, Operation &
operation) method can infinite-loop when slice == 0 because the inner for-loop
never advances the iterator; add the same guard as in take(i, j, step) to check
slice == 0 at the start of each (or throw/assert) and handle it (e.g. return
immediately or raise an error). Locate the each function (uses
const_me()->get_it(pos), it.next(), it.has_curr()) and add a guard at the top to
prevent slice == 0 before entering the while(true) loop.
- Around line 788-796: Add a Doxygen comment block for the primary template
method mutable_for_each(Operation&), documenting its purpose and behavior;
include `@brief` describing that it applies a mutable operation to each T element
via me()->traverse, an `@param` for the Operation& (what it should accept and that
it may modify items), an `@return` (e.g., void) and an `@throws` describing any
exceptions that may propagate from the operation or traverse calls; place the
comment immediately above the mutable_for_each template declaration so it covers
the primary overload.
- Around line 1302-1315: The partition member function mistakenly declares an
unused template parameter Operation, making the overload non-deducible; remove
the spurious "template <class Operation>" so the signature becomes a
non-template member function partition(size_t n) const, leaving the body (use of
const_me(), ret_val, Aleph::DynList<T>, i, and the lambda) unchanged; ensure no
other callers rely on an explicit template parameter for partition and rebuild
to confirm the function is callable as container.partition(n).
- Line 994: The doc comment for foldl contains an incorrect pseudo-code line
"acc = op(acc, op(acc, item);" — update the comment in the foldl documentation
(referencing the foldl function) to the correct expression "acc = op(acc,
item);" by removing the nested op call and fixing the unmatched parenthesis so
the doc matches the implementation.
In `@Examples/cut_nodes_example.C`:
- Around line 373-381: paint_subgraphs() returns the next unused color, so using
its return value as the component count is off-by-one; update the code that
reads long num_colors = compute.paint_subgraphs() to treat num_colors as
next_color (i.e., use num_colors - 1 when displaying "Number of components") and
change the loop to iterate from color = 1 while color < num_colors (or
equivalently color <= num_colors - 1) when printing nodes per component;
reference compute.paint_subgraphs(), the num_colors variable, and the for loop
that currently uses <= num_colors.
In `@Tests/geom_algorithms_test_common.h`:
- Around line 34-36: The function circumcenter_of computes a denominator den
(twice the signed area) and currently divides by it unguarded; add a check in
circumcenter_of immediately before the division that tests if fabs(den) (or den
== 0) is below a small epsilon and, if so, raise an Aleph error macro (e.g.
ALEPH_ERROR or the project’s standard error macro) with a clear message like
"collinear triangle in circumcenter_of" to prevent the division-by-zero; apply
the same guard to the other occurrence of the same code block (lines ~155-174)
so both divisions are protected.
---
Nitpick comments:
In `@Examples/heavy_light_decomposition_example.cc`:
- Around line 143-210: Replace all uses of std::printf in this example with
C++20 type-safe formatting using std::format combined with std::cout (or
std::puts if you want a newline-only). Specifically, update the prints around
hld.size()/labels/hld.position_of_id, the "Path maintenance cost queries" loop
that prints q.name/ans/brute, the subtree totals loop that prints
labels[id]/ans, the budget change messages, the "North -> S-A2 after update"
print that follows grid.update_node_id/grid.set_node_id, and the
hld.for_each_path_segment print; include <format> and construct formatted
strings via std::format(...) then output with std::cout << formatted << '\n' to
replace each std::printf call. Ensure types/format specifiers are adjusted to
std::format syntax (use {} placeholders) and keep existing variables like
id_sa2/id_west, na/sa2, grid.query_path_id, brute_path_sum unchanged.
In `@Examples/weighted_blossom_example.cc`:
- Around line 55-64: The example currently uses std::vector, std::set,
std::tuple (and a global "using namespace std;"); replace those STL container
usages with the corresponding Aleph container types and headers wherever an
equivalent exists and remove the "using namespace std;" directive; update all
occurrences of std::vector, std::set, std::tuple (and any unqualified container
names that relied on the using directive) to the Aleph equivalents (and include
the appropriate Aleph headers) so the file conforms to the "MUST use Aleph
containers" guideline while keeping STL only for types with no Aleph
counterpart.
In `@Tests/blossom_test.cc`:
- Around line 48-56: Replace the GCC/Clang builtin uses of __builtin_ctzll with
the C++20 standard std::countr_zero and add `#include` <bit>; specifically, update
the two call sites that currently call __builtin_ctzll(...) to
std::countr_zero(...) (ensuring the argument type is an unsigned integer type
compatible with std::countr_zero, e.g., static_cast<unsigned long long>(... ) if
necessary) and add the <bit> include at the top of Tests/blossom_test.cc so the
code uses the portable C++20 API.
In `@Tests/hld_test.cc`:
- Around line 66-100: The duplicated test helpers build_graph_with_unit_arcs,
Positive_Arc_Filter, and make_random_tree_edges appear in multiple test files;
extract these into a single shared test utility header (e.g.,
tests/utilities/test_graph_helpers.hpp) and replace the local copies by
including that header in lca_test.cc, hld_test.cc, and
tree_decomposition_test.cc. Move the template function
build_graph_with_unit_arcs, the struct Positive_Arc_Filter, and the
make_random_tree_edges implementation into the new header within the Aleph (or
anonymous) test namespace, ensure all required headers/types are included or
forward-declared there, and update the test files to remove the duplicates and
add a single `#include` for the new header. Ensure symbol names
(build_graph_with_unit_arcs, Positive_Arc_Filter, make_random_tree_edges) and
their signatures remain unchanged so existing tests compile without other
changes.
lrleon
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CMakeLists.txt (1)
82-126:⚠️ Potential issue | 🔴 Critical
Dominators.HandHungarian.Hare missing from the installed header list.Both
Dominators.HandHungarian.Hexist in the repository and are new public headers with full APIs, but they are not listed inHLIST. They will not be installed viainstall(FILES ${HLIST} ...), making these APIs unavailable to downstream consumers of the library.Proposed fix
LCA.H HLD.H + Dominators.H + Hungarian.H )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 82 - 126, HLIST is missing the new public headers Dominators.H and Hungarian.H so they aren't installed; add "Dominators.H" and "Hungarian.H" to the HLIST variable (the list used by install(FILES ${HLIST} ...)) so those headers are packaged/installed for downstream consumers, ensuring you insert the names alongside the other .H entries in the HLIST definition.
🟡 Minor comments (10)
Tests/ah-functional.cc-1490-1491 (1)
1490-1491:⚠️ Potential issue | 🟡 Minor
toloweron plaincharis undefined behavior for non-ASCII values.
charis signed on most platforms; passing a negative value totoloweris UB per C/C++ standard. The tests only use ASCII strings today, but the struct is reusable. Cast tounsigned charbefore callingtolower.🐛 Proposed fix
- if (tolower(a[i]) != tolower(b[i])) return false; + if (tolower(static_cast<unsigned char>(a[i])) != + tolower(static_cast<unsigned char>(b[i]))) return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ah-functional.cc` around lines 1490 - 1491, The loop compares characters using tolower on plain char (variables a and b) which is undefined for negative values; change the calls to cast each character to unsigned char before calling tolower — i.e., replace tolower(a[i]) and tolower(b[i]) with tolower((unsigned char)a[i]) and tolower((unsigned char)b[i]) in the loop that iterates over a.size() so the comparison uses defined behavior..github/workflows/planarity_gephi_nightly.yml-504-512 (1)
504-512:⚠️ Potential issue | 🟡 MinorWebhook secret is passed as a command-line argument — visible in process listings.
${ALERT_WEBHOOK}(sourced from a secret) is passed via--webhook-urlCLI flag. On multi-tenant runners, CLI arguments are visible in/proc/*/cmdlineorps aux. The secret is already available as an env varALERT_WEBHOOK; have the Ruby script read it directly from the environment instead.🛡️ Proposed fix
ruby scripts/planarity_gephi_regression_notify.rb \ --report-json build-gephi-nightly-summary/gephi_nightly_comparison.json \ --output-md build-gephi-nightly-summary/gephi_nightly_alert.md \ --run-url "${RUN_URL}" \ --repository "${REPOSITORY}" \ - --webhook-url "${ALERT_WEBHOOK}" \ + --webhook-url-env ALERT_WEBHOOK \ --print-summaryThen update the Ruby script to read the URL from
ENV['ALERT_WEBHOOK']when--webhook-url-envis specified.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/planarity_gephi_nightly.yml around lines 504 - 512, Do not pass the secret via CLI; instead export ALERT_WEBHOOK as an environment variable to the Ruby process and modify the script to accept a switch that reads from ENV. Update the workflow invocation of scripts/planarity_gephi_regression_notify.rb to remove the --webhook-url "${ALERT_WEBHOOK}" argument and ensure ALERT_WEBHOOK is injected into the job environment; then change the Ruby script to support a new flag (e.g., --webhook-url-env) and when that flag is present have the script read ENV['ALERT_WEBHOOK'] rather than relying on a CLI --webhook-url value..github/workflows/planarity_gephi_nightly.yml-259-264 (1)
259-264:⚠️ Potential issue | 🟡 MinorAddress Python 3.12+
tarfile.extractall()deprecation and harden archive extraction.The
tarfile.extractall()call emits aDeprecationWarningin Python 3.12+ when thefilterparameter is omitted and will change to safer defaults in Python 3.14. Addfilter='data'to strip dangerous path components.For
zipfile.extractall(), while the stdlib module has built-in protections against path traversal, the official Python documentation still recommends validating archive member paths for defense-in-depth. Consider adding a path validation check before extraction:if asset_name.lower().endswith(".zip"): with zipfile.ZipFile(archive_path, "r") as zf: for info in zf.infolist(): target = (ex_dir / info.filename).resolve() if not str(target).startswith(str(ex_dir.resolve())): raise SystemExit(f"Zip entry escapes target dir: {info.filename}") zf.extractall(ex_dir) elif asset_name.lower().endswith(".tar.gz"): with tarfile.open(archive_path, "r:gz") as tf: tf.extractall(ex_dir, filter='data')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/planarity_gephi_nightly.yml around lines 259 - 264, The archive extraction code using zipfile.ZipFile(...) and tarfile.open(..., "r:gz") must be hardened and updated for Python 3.12+; before calling zf.extractall(ex_dir) validate each zip member path by resolving (ex_dir / info.filename).resolve() and ensure it starts with ex_dir.resolve(), raising an error if it escapes the target directory, and for tarfile use tf.extractall(ex_dir, filter='data') to avoid the DeprecationWarning and strip dangerous path components; update the extraction branches that check asset_name.lower().endswith(".zip") and asset_name.lower().endswith(".tar.gz") to implement these changes around archive_path, zf, and tf.Tests/cut_nodes_test.cc-1069-1086 (1)
1069-1086:⚠️ Potential issue | 🟡 Minor
FreeFunctionConsistentWithClass: free-function result's arc identity is never verified.The test confirms
b_classcontainspend(line 1085) but only checks thatb_free.size() == b_class.size(). Ifb_freereturns a different arc of the same count, the test passes silently.🐛 Proposed fix
EXPECT_EQ(b_class.size(), 1U); EXPECT_TRUE(bridge_set(b_class).exist(pend)); + EXPECT_TRUE(bridge_set(b_free).exist(pend));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/cut_nodes_test.cc` around lines 1069 - 1086, The test FreeFunctionConsistentWithClass currently only checks sizes, so add an identity check that the free-function result actually contains the same arc as the class result: after computing b_free with find_bridges(g, nodes[0]) assert that bridge_set(b_free).exist(pend) (or compare bridge_set(b_free) == bridge_set(b_class)) so the exact arc pend is present; update the assertions in the TEST(Bridges, FreeFunctionConsistentWithClass) to verify b_free contains pend in addition to the size checks.Dominators.H-510-522 (1)
510-522:⚠️ Potential issue | 🟡 MinorDominance frontier computation may produce duplicate entries.
When multiple predecessors of a join point
bshare an ancestor on the walk up toidom(b),bnodeis appended to that ancestor's frontier list multiple times. SinceDynListis used rather than a set, the frontier can contain duplicates.This doesn't break SSA phi-placement (placement is idempotent) but violates the set semantics documented for DF.
Proposed fix: use a set to track already-added nodes
// Walk up from each predecessor to idom(b) + DynSetTree<long> already_in_df; for (auto it = preds.get_it(); it.has_curr(); it.next_ne()) { long runner = it.get_curr(); while (runner != idom_arr[b] and runner != -1) { - df.find(vertex[runner]).append(bnode); + if (not already_in_df.contains(runner)) + { + already_in_df.insert(runner); + df.find(vertex[runner]).append(bnode); + } runner = idom_arr[runner]; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dominators.H` around lines 510 - 522, The current dominance-frontier loop appends bnode for each ancestor visited and can add duplicates when multiple predecessors share ancestors; modify the loop in the computation that iterates preds, idom_arr, vertex and df so it tracks which runner nodes have already had bnode added for this b (e.g., create a local temporary set or bitmap "seen" cleared per b) and only call df.find(vertex[runner]).append(bnode) when seen does not contain runner, then mark runner as seen; this preserves DF as a set and avoids duplicate entries.Min_Cost_Matching.H-400-433 (1)
400-433:⚠️ Potential issue | 🟡 MinorDocument functor
operator()overloads with full Doxygen tags.The public
operator()methods in both functor wrappers lack@brief,@param,@return, and@throwstags. Please add full Doxygen blocks to satisfy the public API documentation rule. As per coding guidelines “All public member functions must have@brief,@param,@return, and@throwsDoxygen tags.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Min_Cost_Matching.H` around lines 400 - 433, Add full Doxygen comment blocks to both public operator() overloads: the one returning Min_Cost_Matching_Result<long long> and Compute_Minimum_Cost_Perfect_General_Matching::operator() so they include `@brief`, `@param` g (describe graph input), `@param` matching (describe matching output list), `@return` (describe the result type), and `@throws` (list possible exceptions such as allocation or runtime errors). Place the block immediately above each operator() declaration and ensure parameter names match exactly (g, matching) and the `@return` text matches the concrete return types (Min_Cost_Matching_Result<long long> and Min_Cost_Perfect_Matching_Result<long long>).Hungarian.H-344-352 (1)
344-352:⚠️ Potential issue | 🟡 MinorReject initializer_list rows with zero columns.
rows = {{}}yieldsorig_cols_ == 0but the ctor doesn’t guard it, while the DynMatrix ctor rejects empty matrices. Add a check fororig_cols_ == 0to keep behavior consistent.🔧 Proposed fix
orig_rows_ = rows.size(); orig_cols_ = rows.begin()->size(); + ah_invalid_argument_if(orig_cols_ == 0) + << "Hungarian_Assignment: cost matrix must not be empty"; n_ = orig_rows_ > orig_cols_ ? orig_rows_ : orig_cols_;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Hungarian.H` around lines 344 - 352, The constructor Hungarian_Assignment(std::initializer_list<std::initializer_list<Cost_Type>> rows) currently checks rows.size()==0 but not the case where rows contains zero-length inner lists (rows = {{}}), which sets orig_cols_==0; add a guard after computing orig_cols_ (or before setting n_) that uses the same error macro (ah_invalid_argument_if(orig_cols_ == 0) << "Hungarian_Assignment: cost matrix must have at least one column") so the ctor rejects zero-column input consistently with DynMatrix and prevents n_ from being set from an invalid orig_cols_; reference Hungarian_Assignment, orig_cols_, orig_rows_, and n_.Tests/min_cost_matching_test.cc-98-205 (1)
98-205:⚠️ Potential issue | 🟡 MinorPrefer Aleph containers for helper data structures.
These utilities use
std::vectorandstd::unordered_*; project rules require Aleph equivalents (e.g.,DynList,Array,OLhashTable,DynSetTree) when available. Please migrate to Aleph containers for consistency across the test suite. As per coding guidelines “MUST use Aleph containers when an equivalent exists. Standard library containers are ONLY permitted when no Aleph equivalent is available.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/min_cost_matching_test.cc` around lines 98 - 205, Replace standard containers with Aleph equivalents: in build_graph replace std::vector<typename GT::Node *> nodes with Aleph::Array<typename GT::Node*> (or DynList if dynamic push_back semantics are required) and use its insert/at APIs when creating nodes; in verify_matching replace std::unordered_set<typename GT::Arc *> arcs_in_graph and std::unordered_set<typename GT::Node *> used_nodes with Aleph::DynSetTree<typename GT::Arc*> and Aleph::DynSetTree<typename GT::Node*> respectively (use GT::Arc_Iterator and matching iterator the same way); in normalize_simple_edges replace std::unordered_map<Pair,long long,Pair_Hash> best with Aleph::OLhashTable<Pair,long long,Pair_Hash> and replace result std::vector<Cost_Edge> with an Aleph::DynList<Cost_Edge> or Aleph::Array<Cost_Edge> (choose DynList if you rely on push_back semantics), preserving the same logic in build_graph, verify_matching, better and normalize_simple_edges but using Aleph container APIs and constructors instead of std::. Ensure to update any reserve/populate/iteration calls to the Aleph equivalents and include the corresponding Aleph headers.Tests/geom_algorithms_test_common.h-34-174 (1)
34-174:⚠️ Potential issue | 🟡 MinorUse Aleph error macros for degeneracy checks.
circumcenter_of()throwsstd::domain_errordirectly. The project requires Aleph error macros; please switch toah_domain_error_ifand includeah-errors.H.🔧 Proposed fix
-#include <stdexcept> +#include <ah-errors.H> @@ - if (den == 0) - throw std::domain_error("circumcenter_of: collinear triangle (zero area)"); + ah_domain_error_if(den == 0) + << "circumcenter_of: collinear triangle (zero area)";As per coding guidelines “ALWAYS use Aleph exception macros from ah-errors.H instead of raw throw statements.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/geom_algorithms_test_common.h` around lines 34 - 174, The circumcenter_of function currently throws std::domain_error directly for the collinear/zero-area check; change this to use the Aleph error macro by including ah-errors.H and replacing the throw with ah_domain_error_if(den == 0, "circumcenter_of: collinear triangle (zero area)"); locate the check in circumcenter_of (function name) and remove the raw throw, and add the include for ah-errors.H near the other includes at the top of the file.Examples/min_mean_cycle_example.cc-69-122 (1)
69-122:⚠️ Potential issue | 🟡 MinorPersistent
coutstate mutation leaks out ofprint_result; usestd::format.
cout << fixed << setprecision(6)(line 81) is set inside the function body after the early return, so it only fires when a cycle exists. Becausecoutformatting flags are global, any subsequent output — including the value-only block at line 161 — silently inheritsfixed/setprecision(6). If the first call hits the early-return path (no cycle), the laterminimum_meanprint gets the default (un-formatted) representation.Per coding guidelines, prefer
std::formatfor type-safe string formatting over stream manipulators.♻️ Proposed refactor using `std::format`
+#include <format> ... void print_result(const Graph & g, const string & title, const Min_Mean_Cycle_Result<Graph, double> & r) { - cout << title << '\n'; + cout << std::format("{}\n", title); if (not r.has_cycle) { - cout << " no directed cycle\n\n"; + cout << " no directed cycle\n\n"s; return; } - cout << fixed << setprecision(6); - cout << " minimum mean = " << r.minimum_mean << '\n'; + cout << std::format(" minimum mean = {:.6f}\n", r.minimum_mean); if (r.cycle_length == 0) { - cout << " witness cycle unavailable\n\n"; + cout << " witness cycle unavailable\n\n"s; return; } - const long double witness_mean = static_cast<long double>(r.cycle_total_cost) - / static_cast<long double>(r.cycle_length); - cout << " witness cycle: cost=" << r.cycle_total_cost - << ", length=" << r.cycle_length - << ", mean=" << witness_mean << '\n'; + const double witness_mean = r.cycle_total_cost + / static_cast<double>(r.cycle_length); + cout << std::format(" witness cycle: cost={:.6f}, length={}, mean={:.6f}\n", + r.cycle_total_cost, r.cycle_length, witness_mean); ...As per coding guidelines: "Use
std::formatfor type-safe string formatting (prefer over printf/streams)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/min_mean_cycle_example.cc` around lines 69 - 122, The function print_result mutates global cout formatting with "cout << fixed << setprecision(6)" which leaks outside the function; replace stream manipulators with std::format-based, local formatting. Remove the cout << fixed << setprecision(6) line and format numeric outputs (r.minimum_mean and witness_mean, and any numeric arc/node info) using std::format with a 6-decimal format specifier (e.g. std::format("{:.6f}", value)), then pass those formatted strings to cout (or build a single formatted string for each line); ensure you `#include` <format> and keep all other logic in print_result (use r.cycle_total_cost, r.cycle_length, r.minimum_mean, r.cycle_nodes, r.cycle_arcs, g.get_src_node, g.get_tgt_node, arc->get_info(), node->get_info() to locate places to change).
🧹 Nitpick comments (11)
Tests/ah-functional.cc (2)
1499-1501: Preferstd::absover unqualifiedfabsfor C++20 style.
fabsis the C<math.h>function.std::absfrom<cmath>has afloatoverload in C++ and is the idiomatic choice.♻️ Proposed fix
- return fabs(a - b) <= tolerance; + return std::abs(a - b) <= tolerance;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ah-functional.cc` around lines 1499 - 1501, In the comparator operator()(float a, float b) const replace the C fabs call with the C++ idiomatic std::abs (i.e., use std::abs(a - b) <= tolerance) and ensure <cmath> is included so the float overload is available; update the body of operator() accordingly and qualify the call with std:: to avoid relying on unqualified lookup.
1546-1554:custom_predicate_with_rvalueduplicates existing tests without distinct coverage.Both assertions mirror
case_insensitive_strings_falseandcase_insensitive_strings_trueexactly. If the intent is to test passing the container as an rvalue (e.g.,all_unique(build_dynlist<string>(...), ...)), the test should be updated accordingly; otherwise it can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ah-functional.cc` around lines 1546 - 1554, The test all_unique_overload::custom_predicate_with_rvalue currently duplicates existing cases; update it to actually exercise the rvalue overload by calling all_unique with a temporary container (e.g., all_unique(build_dynlist<string>("Apple","apple","Banana"), CaseInsensitiveStringEqual()) and similarly for the true case), ensuring you use the build_dynlist call directly rather than a named variable, or delete the test if you prefer not to cover the rvalue overload; reference the test name all_unique_overload, the all_unique function, build_dynlist, and CaseInsensitiveStringEqual when making the change..github/workflows/planarity_gephi_nightly.yml (4)
37-40: Unused Ruby setup inprepare-tag-matrixjob.This job only runs an inline Python script. The Ruby setup adds unnecessary CI time (~15-30s) with no benefit. Consider removing it.
♻️ Proposed fix
- - name: Setup Ruby - uses: ruby/setup-ruby@v1 - with: - ruby-version: '3.3' -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/planarity_gephi_nightly.yml around lines 37 - 40, The "Setup Ruby" step (uses: ruby/setup-ruby@v1, with ruby-version: '3.3') inside the prepare-tag-matrix job is unused because the job only runs an inline Python script; remove that step from the prepare-tag-matrix job so the workflow no longer installs Ruby unnecessarily and reduces CI time.
86-94: GitHub API requests are unauthenticated — subject to 60 req/hour rate limit.Both the tag-resolution step (here) and the per-matrix download step (line 188) call the GitHub API without an
Authorizationheader. Unauthenticated requests share a 60 req/hr budget per IP. For a weekly schedule this is likely fine, but if runners share IPs or other workflows run concurrently, it could fail. Consider passingGITHUB_TOKENfor the 5,000 req/hr authenticated limit.♻️ Suggested: add token header
req = urllib.request.Request( "https://api.github.com/repos/gephi/gephi/releases?per_page=100", headers={ "Accept": "application/vnd.github+json", "User-Agent": "aleph-planarity-gephi-nightly", + "Authorization": f"Bearer {os.environ.get('GITHUB_TOKEN', '')}", }, )And add
GITHUB_TOKENto the step's env:env: MANUAL_GEPHI_TAGS: ${{ github.event.inputs.gephi_tags }} GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}Same pattern applies to the download step at line 188.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/planarity_gephi_nightly.yml around lines 86 - 94, The GitHub API requests created with urllib.request.Request (variable req) are unauthenticated and risk hitting the 60 req/hr limit; update the request headers to include an Authorization header using the GITHUB_TOKEN (e.g., "Authorization": f"token {GITHUB_TOKEN}") and ensure the workflow step exposes GITHUB_TOKEN in the step env (add GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} alongside MANUAL_GEPHI_TAGS). Apply the same header change to the other GitHub API request used in the per-matrix download step (the call referenced around line 188) so both requests use the authenticated 5,000 req/hr quota.
406-420: Mutatingproc.returncodeis fragile — use a dedicated exit-code variable.Line 410 reassigns
proc.returncode = 2after the process has completed, which is unusual and could confuse future readers. A separate variable makes the intent clearer and avoids coupling to theCompletedProcessobject.♻️ Proposed refactor
+ exit_code = proc.returncode if proc.returncode == 0: payload = json.loads(proc.stdout) summary["overall_valid"] = bool(payload.get("overall_valid", False)) if not summary["overall_valid"]: - proc.returncode = 2 + exit_code = 2 (out_dir / "nightly_case_summary.json").write_text( json.dumps(summary, indent=2, sort_keys=True) + "\n", encoding="utf-8", ) - if proc.returncode != 0: + if exit_code != 0: print(proc.stdout) print(proc.stderr, file=sys.stderr) - raise SystemExit(proc.returncode) + raise SystemExit(exit_code)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/planarity_gephi_nightly.yml around lines 406 - 420, The code mutates proc.returncode after the process completed; instead introduce a dedicated exit_code variable (e.g., exit_code = proc.returncode) right after obtaining proc, use payload to set summary["overall_valid"], and if not summary["overall_valid"] set exit_code = 2 (not proc.returncode); continue to write nightly_case_summary.json from summary and then check if exit_code != 0 to print proc.stdout/proc.stderr and raise SystemExit(exit_code). This keeps proc immutable and makes intent explicit.
539-543: Minor script-injection risk from direct${{ }}interpolation in bash.Line 542 interpolates
${{ steps.compare.outputs.num_regressions }}directly into a bash script. If the output contained shell metacharacters, they would be interpreted. While the value originates from a controlled Ruby script (likely just a number), the safer pattern is to pass it through an environment variable.🛡️ Proposed fix
- name: Enforce regression gate if: ${{ steps.compare.outputs.has_regressions == 'true' }} shell: bash + env: + NUM_REGRESSIONS: ${{ steps.compare.outputs.num_regressions }} run: | set -euo pipefail echo "Gephi nightly comparison detected regressions across tags." >&2 - echo "num_regressions=${{ steps.compare.outputs.num_regressions }}" >&2 + echo "num_regressions=${NUM_REGRESSIONS}" >&2 exit 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/planarity_gephi_nightly.yml around lines 539 - 543, The run block currently interpolates steps.compare.outputs.num_regressions directly into the shell which can allow shell metacharacters to be interpreted; instead export the output as an environment variable and reference the variable inside the run script (e.g., add an env mapping for NUM_REGRESSIONS: ${{ steps.compare.outputs.num_regressions }} and then echo "$NUM_REGRESSIONS" inside the run block), ensuring you quote the variable and keep set -euo pipefail in place; update references to steps.compare.outputs.num_regressions in this job to use the new NUM_REGRESSIONS env var.K_Shortest_Paths.H (2)
192-209: Thread-local static state inReverse_Arc_Distanceis not re-entrant.
map_ptrandbase_ptrarethread_localstatics, making them safe across threads but not re-entrant within the same thread. Ifbuild_suffix_index_digraphwere ever called from within a callback during another k-shortest-paths computation on the same thread, the pointers would be overwritten. The RAII guardReset_Reverse_Distance(line 547) helps but doesn't prevent nested use.This is an existing pattern in the codebase (cookie-based approaches have similar limitations), so it's acceptable as-is. A brief
@warningnoting non-re-entrant usage would be valuable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@K_Shortest_Paths.H` around lines 192 - 209, Reverse_Arc_Distance currently uses thread_local static pointers (map_ptr, base_ptr) which are not re-entrant within the same thread; add a brief documentation warning to the Reverse_Arc_Distance declaration (and/or the Reset_Reverse_Distance RAII guard) stating that build_suffix_index_digraph and any use of Reverse_Arc_Distance is not re-entrant on the same thread and must not be invoked nested (mention map_ptr and base_ptr by name and reference Reset_Reverse_Distance for the existing guard) so callers are aware of the limitation.
238-267:get_node_at/get_arc_atare O(n) per call, used in inner loops.These functions perform linear traversal on
DynListto reach an index. They're called insidecompose_candidate,compose_general_candidate, and the Yen spur iteration loop, making per-spur-iteration cost O(path_length²) rather than O(path_length).For typical usage (modest path lengths and k), this is unlikely to be a bottleneck. If performance matters, consider caching a random-access snapshot (e.g.,
Array) of the path's nodes/arcs at the start of each iteration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@K_Shortest_Paths.H` around lines 238 - 267, get_node_at and get_arc_at perform O(n) linear scans and are called repeatedly in inner loops (see compose_candidate, compose_general_candidate, and the Yen spur iteration), causing quadratic cost per spur; fix by creating a random-access snapshot (e.g., an Array or std::vector) of the path's nodes and arcs once at the start of each compose_candidate/compose_general_candidate call and each Yen spur iteration, then replace calls to get_node_at/get_arc_at with direct indexed access into that snapshot (or iterate with a single indexed loop) to make per-path traversal linear.Dominators.H (1)
169-185: Recursive DFS may overflow the stack on deep graphs.
dfs()uses unbounded recursion proportional to the longest DFS path. For large graphs (or pathological ones like long chains), this can cause a stack overflow. The same concern applies tocompress()on line 195, though path compression makes deep chains less likely there.Consider converting to an iterative DFS using an explicit stack if you expect this to be used on very large graphs (100k+ nodes). For a compiler SSA pass on typical function-level CFGs this is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dominators.H` around lines 169 - 185, The recursive dfs function (dfs) can overflow for deep graphs; replace it with an iterative DFS using an explicit stack to perform the same pre-order numbering and initialization of NODE_COUNTER, num_reachable, vertex, par, semi, label_arr and anc, and traverse edges via Out_Iterator/ get_tgt_node while pushing child frames instead of recursing; ensure the iterative logic sets par[v_num] to the parent index and only visits nodes with NODE_COUNTER == -1. Also make compress iterative (compress) to avoid recursion in deepest ancestor chains, applying iterative path-compression on anc/label_arr as in the current recursive logic..github/workflows/ci.yml (1)
236-241: Hardcoded/tmpoutput paths are fragile for CI.The example writes to
/tmp/planarity_k33_certificate.graphmland.gexf, and the CI step checks those exact paths. If the example changes its output directory, this step will fail without a clear error message pointing to the root cause.Consider passing an output directory argument to the example or setting an environment variable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 236 - 241, The CI step "Generate certificate exchange artifacts" hardcodes /tmp output files; update the invocation of ./build-planarity-visual/Examples/planarity_test_example to accept an explicit output directory (or use an environment variable like PLANARITY_OUT_DIR) and write artifacts there instead of /tmp, then change the test checks to look for $PLANARITY_OUT_DIR/planarity_k33_certificate.graphml and $PLANARITY_OUT_DIR/planarity_k33_certificate.gexf (or the chosen output path variable); ensure the workflow exports the variable before running the example and that the example binary (or its wrapper) reads the arg/ENV and writes to that directory so CI is no longer tied to /tmp.Examples/min_mean_cycle_example.cc (1)
90-95: Add a comment clarifying the relationship betweenwitness_meanandr.minimum_mean.
witness_meanrecomputes the mean from the witness cycle's raw cost and length, whiler.minimum_meanis the value returned by the algorithm. A reader cannot tell whether these are expected to be equal or are showing two distinct quantities. A one-line comment would make the intent clear (e.g., "sanity-check: witness cycle mean should equal the reported minimum mean").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/min_mean_cycle_example.cc` around lines 90 - 95, Add a one-line comment above the computation/printing of witness_mean clarifying that witness_mean is recomputed from the witness cycle's raw cost and length and is intended as a sanity-check against r.minimum_mean (i.e., the algorithm's reported minimum mean), so readers know whether they should expect these to match or could differ.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/planarity_gephi_nightly.yml:
- Around line 230-234: The "win" substring check in the Windows branch (where
runner_os == "Windows") falsely matches "darwin"; update the condition that
currently uses '"win" in lower' to a precise check such as checking for "win-"
or "win64" tokens or using lower.startswith("win") or a word-boundary regex
(e.g., r'\bwin\b') against the variable lower before adding to score, so only
true Windows asset names increment score (refer to runner_os, lower, and score
in the diff).
In `@Examples/k_shortest_paths_example.cc`:
- Around line 59-79: Replace the STL containers in Scenario and build_graph with
Aleph equivalents: change Scenario::edges from
std::vector<std::tuple<size_t,size_t,long long>> to an Aleph container (e.g.,
aleph::DynArray of aleph::Array or a small Edge struct) and change the nodes
buffer in build_graph from std::vector<Graph::Node*> to an Aleph
DynArray/DynList; update build_graph to reserve/append using the Aleph API and
iterate accessing tuple elements via array indices or the Edge struct instead of
std::tuple structured bindings. Ensure all uses of Scenario::edges and nodes
(inserting arcs and nodes) are updated to the Aleph types and methods
(insert/push_back semantics) so no std::vector/std::tuple remain.
In `@Examples/weighted_blossom_example.cc`:
- Around line 83-113: Replace all std::vector/std::set usage in Scenario,
Solve_Output, and build_graph with the corresponding Aleph containers: use
aleph::DynArray for fixed-indexable sequences (nodes and matched_pairs),
aleph::DynArray or aleph::DynList for edges depending on whether random access
is required, and aleph::DynList/DynSetTree where a list or set semantics are
intended; specifically update struct Scenario (nodes, edges), struct
Solve_Output (matched_pairs), and the local nodes container inside template
build_graph<GT> to use aleph containers (e.g., aleph::DynArray<typename
GT::Node*> for the nodes buffer and aleph::DynArray/aleph::DynList for edges),
and adjust any iterator/index usage accordingly so the code compiles with the
Aleph container APIs.
In `@Hungarian.H`:
- Around line 492-508: The template hungarian_max_assignment currently defines
Promoted as std::make_signed_t<std::common_type_t<Cost_Type, long long>>, which
is ill-formed for floating-point Cost_Type; change Promoted to use
std::conditional_t<std::is_floating_point_v<std::common_type_t<Cost_Type, long
long>>, std::common_type_t<Cost_Type, long long>,
std::make_signed_t<std::common_type_t<Cost_Type, long long>>> so floating-point
common types are used as-is while integral types are converted to a signed
promoted type; keep the rest of the function (negated allocation and negation
via static_cast<Promoted>) unchanged.
In `@Tests/blossom_test.cc`:
- Around line 48-123: Replace standard containers in build_graph and
verify_matching with Aleph containers: change the local nodes std::vector to
Aleph::DynArray<typename GT::Node*> in build_graph and adjust its reserve/insert
usage accordingly; change the edges parameter from std::vector to
Aleph::DynArray<std::pair<size_t,size_t>> (or accept both by adding an overload)
so callers supply Aleph arrays. In verify_matching replace arcs_in_graph
std::unordered_set<typename GT::Arc*> with Aleph::OLhashTable<typename GT::Arc*>
(or Aleph::DynSetTree if ordered set semantics are required) and replace
used_nodes std::unordered_set<typename GT::Node*> with
Aleph::DynSetTree<typename GT::Node*>; update iteration/insert/lookup calls to
use the Aleph container APIs while keeping logic in functions build_graph and
verify_matching unchanged.
- Around line 491-520: Add an opt-in deterministic performance regression test
for blossom by creating a new typed test (similar to
DenseRandomStressCrossBackendAgreement) that uses a fixed RNG seed, larger n
(e.g. 500–2000), and constructs the graph via build_graph and runs run_blossom
while measuring elapsed time or operation count; guard it behind an environment
flag or gtest flag (so it doesn't run by default) and assert the
runtime/operation is below a chosen threshold while still verifying correctness
with verify_matching; name the test to reflect perf intent (e.g.,
BlossomMatchingPerfRegression) and keep the measurement deterministic by fixing
the seed and limiting nondeterminism.
In `@Tests/dominators_test.cc`:
- Around line 516-561: Add a deterministic performance regression test for
dominators: create a new opt-in test (e.g., TEST(Dominators,
PerformanceLargeRandomDAG)) or guard an extended run in the existing
LargeRandomDAG with an env var or macro (e.g., PERFORMANCE_TESTS) so it only
runs when enabled; build a larger deterministic DAG (fixed seed RNG, e.g., N ~
5000) and exercise compute_dominators and build_dominator_tree using the same
APIs (compute_dominators, idom_map, build_dominator_tree), measure elapsed
wall-clock time with std::chrono (or count operations if an instrumentation hook
exists), and assert the runtime or operation count is below a chosen budget via
EXPECT_LT/EXPECT_LE to catch regressions; ensure test is deterministic and
documents the opt-in mechanism.
- Around line 38-73: Replace standard containers in make_nodes and idom_map with
Aleph equivalents: change make_nodes to return a DynArray<DG::Node*> (or
DynList<DG::Node*> if preferred) instead of std::vector and use
DynArray::push_back (and reserve if available) to populate it; update its
signature and callers accordingly. Change idom_map to return a
DynMapTree<int,int> instead of std::map<int,int>, and replace
m[node->get_info()] = ... with the appropriate DynMapTree insertion (e.g.,
m.insert(key, value) or m.put(key, value) depending on the API) while keeping
the same iteration over idoms.get_it() and the same logic for mapping
idom->get_info() or -1. Ensure you include the correct Aleph headers/types
(DynArray/DynMapTree/DynList) and update any code that consumes these helpers to
use the Aleph container API.
In `@Tests/k_shortest_paths_test.cc`:
- Around line 337-379: Add an opt-in deterministic performance regression test
for k-shortest paths: create a new TEST (e.g.,
KShortestPathsTest.PerfRegression) that uses fixed RNG seeds and larger graph
sizes than RandomSmallGraphsMatchExactOracle, runs yen_k_shortest_paths and
eppstein_k_shortest_paths (and optionally exact_k_shortest_simple_paths for
smaller baseline) and asserts they finish within a predefined budget (wall-clock
time or operation count) using a guard like PERF_TESTS_ENABLED or an environment
variable to opt-in; ensure the test documents the seed, K, graph generation
routine (random_graph_edges/build_graph) and the exact budget so future changes
can be detected deterministically.
- Around line 61-137: The tests use std::vector and std::unordered_set in
helpers; replace them with Aleph containers per guidelines: use aleph::DynArray
(or aleph::DynList if list semantics are needed) for vectors in
Canonical_Path.nodes, Built_Graph.nodes, extract_node_ids return type and the
local ids/ out arrays in normalize_results/build_graph, and replace
std::unordered_set<Node*> in is_simple_path with the Aleph OLhashTable or
DynSetTree variant for pointer membership; update function signatures and local
iterators that reference Canonical_Path, Built_Graph, build_graph,
extract_node_ids, is_simple_path, and normalize_results accordingly so all
uses/initializations and comparisons use the Aleph container APIs instead of
std::vector/unordered_set while preserving the same equality/ordering logic in
operator== and the sort comparator.
In `@Tests/lca_test.cc`:
- Around line 482-547: Add a deterministic performance regression test that
exercises large trees and asserts it completes within a budgeted time or
operation count; create a new TYPED_TEST (e.g., LcaTypedTest.PerfRegression or a
separate test fixture LcaPerfTest) that uses a fixed rng seed, builds a large
tree (e.g., n = 100000 with make_random_tree_edges and
build_graph_with_unit_arcs), constructs Binary_Lifting_LCA<Graph> and
Euler_RMQ_LCA<Graph>, then runs a large number of LCA and distance queries
(reusing node_pick from RandomTreesAgainstNaiveOracle) while measuring elapsed
time with std::chrono and ASSERT_LT(elapsed_ms, kBudgetMs) or a
counted-operations assert; guard the test behind a compile-time or runtime
opt-in (e.g., preprocessor macro LCA_PERF_TEST or a GoogleTest filter) so it is
disabled by default, and reference the existing symbols Binary_Lifting_LCA,
Euler_RMQ_LCA, Naive_Tree_Oracle, make_random_tree_edges, and
build_graph_with_unit_arcs to locate where to add it.
- Around line 66-146: Replace all std::vector usages in Naive_Tree_Oracle with
Aleph containers: change the constructor parameter from const
std::vector<std::pair<size_t,size_t>> & edges to the Aleph equivalent (e.g.
DynArray<std::pair<size_t,size_t>> or DynList) and update member types parent_,
depth_, tin_, tout_ to DynArray<size_t> and adj_ to DynArray<DynList<size_t>>
(or DynArray<Array<size_t>> as preferred). Update all call sites inside
Naive_Tree_Oracle: use the Aleph container constructor/init (size n_), replace
edges.size() with the Aleph size/length accessor, replace adj_[u].push_back(v)
with the Aleph append/insert method, and adjust iteration over edges and adj_ to
use the Aleph iteration API. Also add the necessary Aleph headers/using
directives and ensure the dfs, ah_runtime_error_if checks and other code refer
to the new types.
In `@Tests/min_cost_matching_test.cc`:
- Around line 741-839: Add a deterministic performance-regression test that
exercises the min-cost-matching backends on a single large, fixed-seed instance
and is disabled by default (e.g., name the test with the DISABLED_ prefix or
guard with a PERF_TESTS macro); construct a large graph (e.g., n ~ 1000–2000 or
other target size) using a fixed std::mt19937_64 seed, reuse the existing edge
construction pattern (calls to Cost_Edge emplace_back as in the tests above),
and call the same entry points used in correctness tests
(exact_minimum_cost_matching_optimum /
exact_minimum_cost_perfect_matching_optimum and solve_backend /
solve_backend_perfect for the List_Backend, SGraph_Backend, AGraph_Backend);
measure elapsed time with std::chrono around the backend calls and assert it
meets a reasonable threshold (or log the duration) so CI can detect regressions
while keeping the test disabled by default.
In `@Tests/planarity_test.cc`:
- Around line 440-479: The tests call run_external_certificate_validator(...)
unconditionally and write to hard-coded /tmp paths; update
run_external_certificate_validator and the tests to (1) use
std::filesystem::temp_directory_path() to build stdout/stderr paths instead of
"/tmp" for portability, and (2) add pre-check guards before asserting rc == 0:
verify find_repo_root() and script existence (already checked in
run_external_certificate_validator) and the specific tool availability flags
used elsewhere (e.g., has_nx, has_gephi_usable or equivalent checks for
Ruby/NetworkX/Gephi) and only assert success when those prerequisites are
present—mirror the conditional patterns used in
ExternalCertificateValidatorNetworkxModeIsPortable and
ExternalCertificateValidatorGephiModeIsPortable and apply them to tests such as
ExternalCertificateValidatorCanListGephiTemplates,
ExternalCertificateValidatorCanListGephiRenderProfiles and the render tests that
call run_external_certificate_validator.
---
Outside diff comments:
In `@CMakeLists.txt`:
- Around line 82-126: HLIST is missing the new public headers Dominators.H and
Hungarian.H so they aren't installed; add "Dominators.H" and "Hungarian.H" to
the HLIST variable (the list used by install(FILES ${HLIST} ...)) so those
headers are packaged/installed for downstream consumers, ensuring you insert the
names alongside the other .H entries in the HLIST definition.
---
Duplicate comments:
In `@Tests/cut_nodes_test.cc`:
- Around line 938-948: The previous concern about incorrect Aleph-container
usage has been resolved; confirm no change is needed: ensure the helper
bridge_set and the arc collection variable (e.g., DynList<Graph::Arc *> arcs)
use DynSetTree<Graph::Arc *> and DynList<Graph::Arc *>::Iterator respectively,
and leave the bridge_set function and its usage as shown (no modifications
required).
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 236-241: The CI step "Generate certificate exchange artifacts"
hardcodes /tmp output files; update the invocation of
./build-planarity-visual/Examples/planarity_test_example to accept an explicit
output directory (or use an environment variable like PLANARITY_OUT_DIR) and
write artifacts there instead of /tmp, then change the test checks to look for
$PLANARITY_OUT_DIR/planarity_k33_certificate.graphml and
$PLANARITY_OUT_DIR/planarity_k33_certificate.gexf (or the chosen output path
variable); ensure the workflow exports the variable before running the example
and that the example binary (or its wrapper) reads the arg/ENV and writes to
that directory so CI is no longer tied to /tmp.
In @.github/workflows/planarity_gephi_nightly.yml:
- Around line 37-40: The "Setup Ruby" step (uses: ruby/setup-ruby@v1, with
ruby-version: '3.3') inside the prepare-tag-matrix job is unused because the job
only runs an inline Python script; remove that step from the prepare-tag-matrix
job so the workflow no longer installs Ruby unnecessarily and reduces CI time.
- Around line 86-94: The GitHub API requests created with urllib.request.Request
(variable req) are unauthenticated and risk hitting the 60 req/hr limit; update
the request headers to include an Authorization header using the GITHUB_TOKEN
(e.g., "Authorization": f"token {GITHUB_TOKEN}") and ensure the workflow step
exposes GITHUB_TOKEN in the step env (add GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN
}} alongside MANUAL_GEPHI_TAGS). Apply the same header change to the other
GitHub API request used in the per-matrix download step (the call referenced
around line 188) so both requests use the authenticated 5,000 req/hr quota.
- Around line 406-420: The code mutates proc.returncode after the process
completed; instead introduce a dedicated exit_code variable (e.g., exit_code =
proc.returncode) right after obtaining proc, use payload to set
summary["overall_valid"], and if not summary["overall_valid"] set exit_code = 2
(not proc.returncode); continue to write nightly_case_summary.json from summary
and then check if exit_code != 0 to print proc.stdout/proc.stderr and raise
SystemExit(exit_code). This keeps proc immutable and makes intent explicit.
- Around line 539-543: The run block currently interpolates
steps.compare.outputs.num_regressions directly into the shell which can allow
shell metacharacters to be interpreted; instead export the output as an
environment variable and reference the variable inside the run script (e.g., add
an env mapping for NUM_REGRESSIONS: ${{ steps.compare.outputs.num_regressions }}
and then echo "$NUM_REGRESSIONS" inside the run block), ensuring you quote the
variable and keep set -euo pipefail in place; update references to
steps.compare.outputs.num_regressions in this job to use the new NUM_REGRESSIONS
env var.
In `@Dominators.H`:
- Around line 169-185: The recursive dfs function (dfs) can overflow for deep
graphs; replace it with an iterative DFS using an explicit stack to perform the
same pre-order numbering and initialization of NODE_COUNTER, num_reachable,
vertex, par, semi, label_arr and anc, and traverse edges via Out_Iterator/
get_tgt_node while pushing child frames instead of recursing; ensure the
iterative logic sets par[v_num] to the parent index and only visits nodes with
NODE_COUNTER == -1. Also make compress iterative (compress) to avoid recursion
in deepest ancestor chains, applying iterative path-compression on anc/label_arr
as in the current recursive logic.
In `@Examples/min_mean_cycle_example.cc`:
- Around line 90-95: Add a one-line comment above the computation/printing of
witness_mean clarifying that witness_mean is recomputed from the witness cycle's
raw cost and length and is intended as a sanity-check against r.minimum_mean
(i.e., the algorithm's reported minimum mean), so readers know whether they
should expect these to match or could differ.
In `@K_Shortest_Paths.H`:
- Around line 192-209: Reverse_Arc_Distance currently uses thread_local static
pointers (map_ptr, base_ptr) which are not re-entrant within the same thread;
add a brief documentation warning to the Reverse_Arc_Distance declaration
(and/or the Reset_Reverse_Distance RAII guard) stating that
build_suffix_index_digraph and any use of Reverse_Arc_Distance is not re-entrant
on the same thread and must not be invoked nested (mention map_ptr and base_ptr
by name and reference Reset_Reverse_Distance for the existing guard) so callers
are aware of the limitation.
- Around line 238-267: get_node_at and get_arc_at perform O(n) linear scans and
are called repeatedly in inner loops (see compose_candidate,
compose_general_candidate, and the Yen spur iteration), causing quadratic cost
per spur; fix by creating a random-access snapshot (e.g., an Array or
std::vector) of the path's nodes and arcs once at the start of each
compose_candidate/compose_general_candidate call and each Yen spur iteration,
then replace calls to get_node_at/get_arc_at with direct indexed access into
that snapshot (or iterate with a single indexed loop) to make per-path traversal
linear.
In `@Tests/ah-functional.cc`:
- Around line 1499-1501: In the comparator operator()(float a, float b) const
replace the C fabs call with the C++ idiomatic std::abs (i.e., use std::abs(a -
b) <= tolerance) and ensure <cmath> is included so the float overload is
available; update the body of operator() accordingly and qualify the call with
std:: to avoid relying on unqualified lookup.
- Around line 1546-1554: The test
all_unique_overload::custom_predicate_with_rvalue currently duplicates existing
cases; update it to actually exercise the rvalue overload by calling all_unique
with a temporary container (e.g.,
all_unique(build_dynlist<string>("Apple","apple","Banana"),
CaseInsensitiveStringEqual()) and similarly for the true case), ensuring you use
the build_dynlist call directly rather than a named variable, or delete the test
if you prefer not to cover the rvalue overload; reference the test name
all_unique_overload, the all_unique function, build_dynlist, and
CaseInsensitiveStringEqual when making the change.
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2ab377a81
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (7)
Blossom.H (1)
195-222: LCA fallbackreturn bat line 221 is technically unreachable but adds safety.When
find_augmenting_pathcallslca(v, u), both nodes are in the same alternating tree. The first walk fromato the root marks all bases; the second walk frombmust hit one of them before reaching the root. Thereturn bat line 221 acts as a safety net — if it were ever reached, it would indicate a bug in the tree invariants rather than produce a correct result. Consider adding a debug assertion here.💡 Optional: add a debug assertion
b = static_cast<size_t>(parent[static_cast<size_t>(match[b])]); } + // Should be unreachable: both a and b are in the same alternating tree + assert(false && "lca: b-walk reached root without finding visited base"); return b;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Blossom.H` around lines 195 - 222, The final "return b" in lca(size_t a, size_t b) is only a safety fallback and should be asserted as unreachable; add a debug assertion (e.g., assert(false && "lca fallback reached") or a platform-specific unreachable check) just before the return to document the invariant and help catch logic bugs at runtime, referencing the lca function and leaving the fallback return in place for safety in release builds.Tests/dominators_test.cc (2)
705-718: Post-dominatoripdom_maphelper usesstd::mapinstead ofDynMapTree.The dominator-side
idom_map(line 66) was updated to useDynMapTree<int, int>, but the post-dominator counterpart still usesstd::map<int, int>. This inconsistency violates the Aleph container guideline. As per coding guidelines: "MUST use Aleph containers when an equivalent exists."♻️ Proposed fix
- std::map<int, int> ipdom_map(const DynList<std::pair<DG::Node*, - DG::Node*>> & ipdoms) + DynMapTree<int, int> ipdom_map_post(const DynList<std::pair<DG::Node*, + DG::Node*>> & ipdoms) { - std::map<int, int> m; + DynMapTree<int, int> m; for (auto it = ipdoms.get_it(); it.has_curr(); it.next_ne()) { auto [node, ipdom] = it.get_curr(); - m[node->get_info()] = ipdom ? ipdom->get_info() : -1; + m.insert(node->get_info(), ipdom ? ipdom->get_info() : -1); } return m; }Note: All call sites that use
ipdom_map(...)in the PostDominators tests would need to be updated accordingly (usingDynMapTreeAPI for lookups).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/dominators_test.cc` around lines 705 - 718, The ipdom_map helper currently returns a std::map<int,int>; replace it with Aleph's DynMapTree<int,int> so it matches the idom_map pattern: change the function return type and local variable m to DynMapTree<int,int>, insert entries via m.set(key, value) (or the DynMapTree equivalent) while iterating the DynList of std::pair<DG::Node*,DG::Node*> (ipdoms) and return m; update all PostDominators test call sites to use DynMapTree lookup APIs instead of std::map::operator[] or find to accommodate the container change.
686-698:StringTypedGraphtest usesstd::map<std::string, std::string>instead of Aleph container.For consistency with the rest of the test file, consider using
DynMapTree<std::string, std::string>. As per coding guidelines: "UseDynMapTree<K, V>instead ofstd::map<K, V>."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/dominators_test.cc` around lines 686 - 698, Replace the std::map usage in the dominators test with the project's DynMapTree<K,V> container: change the variable declaration m from std::map<std::string,std::string> to DynMapTree<std::string,std::string>, and when populating it in the loop that iterates idoms via idoms.get_it() use DynMapTree's insert/assignment API to set m[node->get_info()] = idom ? idom->get_info() : "none"; leave the EXPECT_EQ checks for m["entry"], m["body"], and m["exit"] unchanged so the assertions use the DynMapTree instead of std::map.Tests/min_cost_matching_test.cc (1)
56-60: Pervasive use ofstd::vector,std::unordered_set, andstd::unordered_mapin test utilities.The test helpers (
build_graph,verify_matching,normalize_simple_edges, the DP oracle, and all test cases) use standard containers extensively. While some uses are justified (e.g., the bitmask-indexedmemotable in the DP oracle needs O(1) random access by index), manystd::vector<Cost_Edge>instances andstd::unordered_setcould be replaced withArray<Cost_Edge>/DynArray<Cost_Edge>and Aleph hash containers. As per coding guidelines: "MUST use Aleph containers when an equivalent exists."Given the scope of changes needed and that this is test infrastructure, consider this for a follow-up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/min_cost_matching_test.cc` around lines 56 - 60, Many test utilities (build_graph, verify_matching, normalize_simple_edges, the DP oracle, and test cases) use std::vector, std::unordered_set, and std::unordered_map; replace these with the project's Aleph container equivalents (Array/ DynArray for vectors and Aleph hash containers for unordered_* where equivalents exist) to comply with the coding guideline. Update types and construction/iteration code in functions build_graph, verify_matching, normalize_simple_edges, the DP oracle (memo usage that needs O(1) random access should keep an Array or DynArray as appropriate), and all test cases to use Array<Cost_Edge>/DynArray<Cost_Edge> instead of std::vector<Cost_Edge>, and replace std::unordered_set/std::unordered_map usages with the Aleph hash set/map types; keep std::vector only where no Aleph equivalent exists and ensure API callsites (emplace/insert, size, iteration, indexing) are adjusted to the Aleph container methods.Examples/planarity_test_example.cc (1)
274-289: File writes lack error checking.The
ofstreamobjects for writing certificate files (JSON, DOT, GraphML, GEXF) are opened and written to without checking for errors. For an example program this is tolerable, but in a production context you'd want to verify that the files were written successfully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/planarity_test_example.cc` around lines 274 - 289, The current write blocks that create ofstream objects (fj, fd, fg, fx) and stream the contents (json, dot, graphml, gexf) do not check whether the files opened or whether the writes succeeded; update each block that opens json_path, dot_path, graphml_path and gexf_path to verify the stream opened (e.g., test fj/is_open or operator bool) and check for write errors after writing (e.g., check fail() or good()), and on error return/throw or log a clear message including the path and the stream error state so failures are detected and propagated instead of silently ignored.Tests/lca_test.cc (1)
117-131: Recursive DFS inNaive_Tree_Oracleis safe for the current test sizes but could overflow on degenerate linear trees at scale.The
dfsmethod is recursive with depth proportional to tree height. For the random trees used in tests (expected O(log n) height), this is fine. However, if the oracle were ever used with a linear chain of 100k nodes, it would blow the stack. Since it's only a test utility and the tree generator produces balanced random trees, this is acceptable as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/lca_test.cc` around lines 117 - 131, The recursive dfs in Naive_Tree_Oracle (function dfs) can overflow for deep/linear trees; replace it with an iterative DFS using an explicit stack to simulate recursion so parent_, depth_, tin_, tout_, and timer_ are set correctly: push a tuple/state for the root (node, parent, depth, entering flag), on entering set parent_[u], depth_[u], tin_[u]=timer_++, iterate adjacency via adj_[u] to push children (skip parent), and on exiting set tout_[u]=timer_-1; ensure you reference and update timer_, adj_, parent_, depth_, tin_, and tout_ consistently to preserve current behavior without using the call stack.Tests/blossom_test.cc (1)
235-261:build_bipartite_graphusesstd::vectorfor node storage — should useDynArray.
left_nodesandright_nodesarestd::vector<typename GT::Node*>but should beDynArray<typename GT::Node*>per project container policy. The same pattern used inbuild_graph(line 79) should be applied here.♻️ Proposed fix to use Aleph containers
template <class GT> GT build_bipartite_graph(size_t left_size, size_t right_size, const std::vector<std::pair<size_t, size_t>> & edges_lr) { GT g; - std::vector<typename GT::Node *> left_nodes; - std::vector<typename GT::Node *> right_nodes; - left_nodes.reserve(left_size); - right_nodes.reserve(right_size); + DynArray<typename GT::Node *> left_nodes; + DynArray<typename GT::Node *> right_nodes; + left_nodes.reserve(left_size); + right_nodes.reserve(right_size); for (size_t i = 0; i < left_size; ++i) - left_nodes.push_back(g.insert_node(static_cast<int>(i))); + left_nodes(i) = g.insert_node(static_cast<int>(i)); for (size_t i = 0; i < right_size; ++i) - right_nodes.push_back(g.insert_node(static_cast<int>(left_size + i))); + right_nodes(i) = g.insert_node(static_cast<int>(left_size + i)); for (const auto & [l, r] : edges_lr) { if (l >= left_size or r >= right_size) continue; - g.insert_arc(left_nodes[l], right_nodes[r], 1); + g.insert_arc(left_nodes(l), right_nodes(r), 1); } return g; }As per coding guidelines: MUST use Aleph containers when an equivalent exists.
DynArray<T>instead ofstd::vector<T>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/blossom_test.cc` around lines 235 - 261, Replace the std::vector usage in build_bipartite_graph: change left_nodes and right_nodes from std::vector<typename GT::Node*> to DynArray<typename GT::Node*> and use DynArray's reserve and push_back operations when populating nodes (same pattern as build_graph). Ensure you reference GT::Node for the element type and update any includes/imports if needed so DynArray is available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/planarity_gephi_nightly.yml:
- Around line 1-6: The workflow name "Planarity Gephi Nightly" and any
artifact/job naming conventions using the "gephi-nightly-*" prefix are
inconsistent with the cron schedule ('30 3 * * 0') which runs weekly; update the
name field to something like "Planarity Gephi Weekly" and rename artifact/job
prefixes from "gephi-nightly-*" to "gephi-weekly-*" (or another
weekly-consistent prefix) so names match the actual cadence while leaving the
existing schedule unchanged.
- Around line 418-422: When calling json.loads(proc.stdout) in the success
branch, wrap the parse in a try/except that catches json.JSONDecodeError (and
optionally ValueError) so non-JSON output from the Ruby validator doesn't raise
an unhandled exception; on parse failure set summary["overall_valid"] = False,
set exit_code = 2 (or preserve the existing non-valid behavior), and log or
include the raw proc.stdout in the diagnostic message so callers can see the
invalid output; update the block around json.loads, proc.stdout,
summary["overall_valid"], and exit_code to implement this handling.
- Around line 509-524: The workflow step passes an empty ALERT_WEBHOOK to the
Ruby notifier, so add a shell guard at the start of the run block to skip
running scripts when ALERT_WEBHOOK is unset or empty; check the ALERT_WEBHOOK
env variable (from the step's env) and exit successfully (or echo a skip
message) if it's blank before invoking planarity_gephi_regression_notify.rb,
ensuring the step truly behaves as an optional webhook.
- Around line 267-268: The code currently calls
urllib.request.urlopen(req_asset, timeout=300) and then
archive_path.write_bytes(resp.read()), which loads the entire archive into
memory; change this to stream the response to disk in chunks instead: open
archive_path for binary writing and iterate reading resp.read(CHUNK_SIZE) (e.g.,
64KB or 1MB) until empty, writing each chunk to the file, or use
shutil.copyfileobj(resp, fileobj, length=CHUNK_SIZE) to avoid buffering the
whole response; keep the same urlopen call and timeout but replace
resp.read()/write_bytes usage with chunked writes to prevent OOM on large
downloads.
- Around line 247-256: The check that currently only rejects assets with
score_asset(ranked[0]) < 0 is too permissive; update the guard to require the
selected asset to be OS-specific by verifying score_asset(ranked[0]) >= 80
(ensuring the +80 OS token was applied) and otherwise raise the same SystemExit
with the assets names; modify the block using ranked, score_asset, assets and
the SystemExit raise to perform this >=80 check before accepting ranked[0].
- Around line 270-277: The tar extraction call using
tarfile.extractall(archive_path, ...) is unsafe and deprecated without an
explicit filter; update the branch handling asset_name.endswith(".tar.gz") so
the TarFile is opened via tarfile.open(archive_path, "r:gz") as tf and call
tf.extractall(ex_dir, filter="data") to enforce the safe data-only extraction
behavior (keep the existing zipfile.extractall for .zip assets and the
SystemExit fallback unchanged); reference tarfile.open and TarFile.extractall to
locate the edit.
In `@Dominators.H`:
- Around line 267-270: The parameter constness in do_compute is misleading
because the function and helpers (dfs, NODE_COUNTER modifications) mutate node
bookkeeping; remove the const_cast and change the signature of do_compute(const
GT & g, Node *root) to accept GT & g, then propagate the same change to all
public methods and free functions that forward graphs (compute_idom, build_tree,
dominance_frontiers, and any related overloads), update callers to pass
non-const GT references, and eliminate the const_cast usage (gptr =
&const_cast<GT &>(g)); ensure NODE_COUNTER writes remain valid against the
non-const GT parameter.
- Around line 530-562: The dominance_frontiers function currently accumulates
duplicates because df is a DynMapTree<Node*, DynList<Node*>> and the walk-up
loop (the loop over pred[b] that calls df.find(vertex[runner]).append(bnode))
may append the same bnode multiple times; change the frontier storage to a set
or prevent duplicates: replace DynList with a set type (e.g., DynSetTree or
DynSet) so df is DynMapTree<Node*, DynSet<Node*>> (or use DynSetTree<Node*,
DynSet<Node*>>), or alternatively, keep DynList but check membership before
append (use df.find(vertex[runner]).contains(bnode) or equivalent) in the inner
loop where runner is walked up; update any return type and consumers of
dominance_frontiers accordingly (symbols: dominance_frontiers, df, pred,
idom_arr, vertex, num_reachable).
- Around line 267-301: The early return in do_compute (after dfs) can leave pred
uninitialized which causes out-of-bounds access in dominance_frontiers; to fix,
allocate and initialize pred (e.g., pred = Array<long>(N, -1L)) along with the
other internal arrays (vertex, par, semi, idom_arr, anc, label_arr) before the
num_reachable <= 1 check so pred is valid for single-node graphs, and/or add a
guard in dominance_frontiers to return immediately if num_reachable <= 1; update
references to pred and ensure idom_arr[0] is still set for the single-node case.
- Around line 714-788: The cache-check in ensure_computed (uses is_computed,
gptr, exit_ptr) is brittle because it compares raw graph pointer (&g) and will
miss logically-equivalent graph copies; change the validity check to compare a
stable graph identity (e.g., a version/id or size/structure fingerprint) or
require callers to pass the same instance and document the precondition; also
document that invert_digraph produces new Arc objects so sa (the arc filter
passed into Lengauer_Tarjan_Post_Dominators and used by dom) must be stateless
or re-bindable—either update class docs to state filters must not rely on
original Arc identity (mention Dft_Show_Arc as safe) or add a step in
ensure_computed to re-map or re-evaluate arc-dependent filters after rev =
invert_digraph(g) using orig_to_rev/rev_to_orig so custom filters that depend on
arc identity are corrected.
In `@Hungarian.H`:
- Around line 492-520: hungarian_max_assignment can still overflow when Promoted
is a signed type equal to long long and a cell equals numeric_limits<long
long>::min(); add a constexpr check and handle that case by negating using a
wider signed intermediate (e.g., __int128) to avoid overflow: inside the loop
that fills negated(i,j) (refer to Promoted, negated, cost.read), use if
constexpr(std::is_signed_v<Promoted>) { auto v =
static_cast<Promoted>(cost.read(i,j)); if (v ==
std::numeric_limits<Promoted>::min()) negated(i,j) =
static_cast<Promoted>(-static_cast<__int128>(v)); else negated(i,j) = -v; } else
{ negated(i,j) = -static_cast<Promoted>(cost.read(i,j)); } ensuring the wider
type is only used as an intermediate to compute the negated value safely.
In `@K_Shortest_Paths.H`:
- Around line 1025-1185: Add full Doxygen for the public API
eppstein_k_shortest_paths and its functor wrapper
Eppstein_K_Shortest_Paths::operator(): include `@brief`, `@param` entries for g,
source, target, k, distance and sa, `@return` describing the
DynList<K_Shortest_Path_Item<...>>, `@throws` for the domain errors (source/target
null) and negative-weight validation, and a `@note` or `@complexity` line with Big-O
time/space bounds mirroring the level of detail used in yen_k_shortest_paths;
also add a short thread-safety statement. Ensure the tags appear immediately
above the template function eppstein_k_shortest_paths and above the operator()
method in class Eppstein_K_Shortest_Paths.
In `@Tests/k_shortest_paths_test.cc`:
- Around line 393-419: Add a new unit test that constructs a small Graph via
make_edges/build_graph containing (1) a self-loop on a spur node and (2)
parallel arcs between the same node pair with different weights, then call
yen_k_shortest_paths<Graph> and eppstein_k_shortest_paths<Graph> from the same
source/target and assert both algorithms return the same K results, that the
self-loop is not chosen as a valid simple path spur, and that parallel edges
appear in the returned path list in deterministic order by total weight (use
EXPECT_EQ/EXPECT_TRUE to check count, ordering, and exact path weights/vertex
sequences). Ensure you reference the same types and functions used elsewhere
(yen_k_shortest_paths, eppstein_k_shortest_paths, make_edges, build_graph,
Graph) so the test integrates with existing tests.
---
Duplicate comments:
In @.github/workflows/planarity_gephi_nightly.yml:
- Around line 236-244: The Windows OS detection conditional using runner_os ==
"Windows" and checks on the variable lower (lower.startswith("win"), "win-" in
lower, "win64" in lower, "win32" in lower) correctly avoids matching "darwin"
and resolves the false-positive; no code change required—please mark this review
as approved/merge-ready and remove any previous darwin-related workarounds that
were introduced to handle the false positive if they exist.
---
Nitpick comments:
In `@Blossom.H`:
- Around line 195-222: The final "return b" in lca(size_t a, size_t b) is only a
safety fallback and should be asserted as unreachable; add a debug assertion
(e.g., assert(false && "lca fallback reached") or a platform-specific
unreachable check) just before the return to document the invariant and help
catch logic bugs at runtime, referencing the lca function and leaving the
fallback return in place for safety in release builds.
In `@Examples/planarity_test_example.cc`:
- Around line 274-289: The current write blocks that create ofstream objects
(fj, fd, fg, fx) and stream the contents (json, dot, graphml, gexf) do not check
whether the files opened or whether the writes succeeded; update each block that
opens json_path, dot_path, graphml_path and gexf_path to verify the stream
opened (e.g., test fj/is_open or operator bool) and check for write errors after
writing (e.g., check fail() or good()), and on error return/throw or log a clear
message including the path and the stream error state so failures are detected
and propagated instead of silently ignored.
In `@Tests/blossom_test.cc`:
- Around line 235-261: Replace the std::vector usage in build_bipartite_graph:
change left_nodes and right_nodes from std::vector<typename GT::Node*> to
DynArray<typename GT::Node*> and use DynArray's reserve and push_back operations
when populating nodes (same pattern as build_graph). Ensure you reference
GT::Node for the element type and update any includes/imports if needed so
DynArray is available.
In `@Tests/dominators_test.cc`:
- Around line 705-718: The ipdom_map helper currently returns a
std::map<int,int>; replace it with Aleph's DynMapTree<int,int> so it matches the
idom_map pattern: change the function return type and local variable m to
DynMapTree<int,int>, insert entries via m.set(key, value) (or the DynMapTree
equivalent) while iterating the DynList of std::pair<DG::Node*,DG::Node*>
(ipdoms) and return m; update all PostDominators test call sites to use
DynMapTree lookup APIs instead of std::map::operator[] or find to accommodate
the container change.
- Around line 686-698: Replace the std::map usage in the dominators test with
the project's DynMapTree<K,V> container: change the variable declaration m from
std::map<std::string,std::string> to DynMapTree<std::string,std::string>, and
when populating it in the loop that iterates idoms via idoms.get_it() use
DynMapTree's insert/assignment API to set m[node->get_info()] = idom ?
idom->get_info() : "none"; leave the EXPECT_EQ checks for m["entry"], m["body"],
and m["exit"] unchanged so the assertions use the DynMapTree instead of
std::map.
In `@Tests/lca_test.cc`:
- Around line 117-131: The recursive dfs in Naive_Tree_Oracle (function dfs) can
overflow for deep/linear trees; replace it with an iterative DFS using an
explicit stack to simulate recursion so parent_, depth_, tin_, tout_, and timer_
are set correctly: push a tuple/state for the root (node, parent, depth,
entering flag), on entering set parent_[u], depth_[u], tin_[u]=timer_++, iterate
adjacency via adj_[u] to push children (skip parent), and on exiting set
tout_[u]=timer_-1; ensure you reference and update timer_, adj_, parent_,
depth_, tin_, and tout_ consistently to preserve current behavior without using
the call stack.
In `@Tests/min_cost_matching_test.cc`:
- Around line 56-60: Many test utilities (build_graph, verify_matching,
normalize_simple_edges, the DP oracle, and test cases) use std::vector,
std::unordered_set, and std::unordered_map; replace these with the project's
Aleph container equivalents (Array/ DynArray for vectors and Aleph hash
containers for unordered_* where equivalents exist) to comply with the coding
guideline. Update types and construction/iteration code in functions
build_graph, verify_matching, normalize_simple_edges, the DP oracle (memo usage
that needs O(1) random access should keep an Array or DynArray as appropriate),
and all test cases to use Array<Cost_Edge>/DynArray<Cost_Edge> instead of
std::vector<Cost_Edge>, and replace std::unordered_set/std::unordered_map usages
with the Aleph hash set/map types; keep std::vector only where no Aleph
equivalent exists and ensure API callsites (emplace/insert, size, iteration,
indexing) are adjusted to the Aleph container methods.
Contributor
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @lrleon. The following files were modified: * `Examples/blossom_example.cc` * `Examples/centroid_decomposition_example.cc` * `Examples/dominators_example.cc` * `Examples/heavy_light_decomposition_example.cc` * `Examples/hld_example.cc` * `Examples/hungarian_example.cc` * `Examples/k_shortest_paths_example.cc` * `Examples/lca_example.cc` * `Examples/min_cost_matching_example.cc` * `Examples/min_mean_cycle_example.cc` * `Examples/planarity_test_example.cc` * `Examples/weighted_blossom_example.cc` * `Tests/ah-functional.cc` * `Tests/geom_algorithms_test_common.h` These files were ignored: * `Tests/ah_concepts_test.cc` * `Tests/blossom_test.cc` * `Tests/cut_nodes_test.cc` * `Tests/dominators_test.cc` * `Tests/hld_test.cc` * `Tests/hungarian_test.cc` * `Tests/k_shortest_paths_test.cc` * `Tests/lca_test.cc` * `Tests/min_cost_matching_test.cc` * `Tests/min_mean_cycle_test.cc` These file types are not supported: * `.github/workflows/ci.yml` * `.github/workflows/planarity_gephi_nightly.yml` * `Blossom.H` * `Blossom_Weighted.H` * `CMakeLists.txt` * `Dominators.H` * `Examples/CMakeLists.txt` * `Examples/cut_nodes_example.C` * `HLD.H` * `Hungarian.H` * `K_Shortest_Paths.H` * `LCA.H` * `Min_Cost_Matching.H` * `Min_Mean_Cycle.H` * `README.es.md` * `README.md` * `Tests/CMakeLists.txt`
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (27)
Examples/dominators_example.cc (1)
161-205: Duplicated frontier-printing logic between sections 4 and 5.Sections 4 (lines 161–176) and 5 (lines 187–204) iterate over
dfwith nearly identical printing loops. For an example file this is tolerable, but extracting a small lambda (e.g.,print_frontier) would reduce the duplication and make both sections easier to follow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/dominators_example.cc` around lines 161 - 205, Duplicate iteration/printing over the dominance-frontier map `df` appears in the two blocks that use `for (auto it = df.get_it(); it.has_curr(); it.next_ne())` and inner `for (auto fi = frontier.get_it(); fi.has_curr(); fi.next_ne())`; refactor by extracting a small reusable lambda or helper (e.g., `print_frontier`) that accepts a node and its `frontier` and prints the "{...}" list with the same comma-separation logic, then replace both duplicated loops (the one under "=== Dominance Frontiers ===" and the one under "=== SSA Phi-Function Placement ===") to call this helper for each `it.get_curr()` pair; ensure the helper uses the same calls (`get_info()`, `is_empty()`, `get_it()`, `has_curr()`, `next_ne()`) so behavior is unchanged.Examples/planarity_test_example.cc (2)
340-412: Consider extracting the verbose helper-command output into a separate function.This ~70-line block of
coutstatements printing validator/render/CI commands obscures the core planarity demonstration. Extracting it (e.g.,print_helper_commands(output_dir, graphml_path, gexf_path)) would keepmainfocused on the graph construction and planarity-test workflow, and make the helper-command catalog easier to maintain independently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/planarity_test_example.cc` around lines 340 - 412, Extract the ~70-line cout block into a new helper function like print_helper_commands(const fs::path& output_dir, const string& graphml_path, const string& gexf_path) and replace the block in main with a single call to that function; move the derived paths (render_dir, ci_report, ci_render_report, visual_render_dir, visual_report, nightly_comparison_json, nightly_alert_md) into the helper so they are computed locally, keep the same printed strings/formatting, and ensure the new function is declared before main (or in an appropriate header) so compilation succeeds.
313-328: File writes are not checked for success — certificate files could silently fail.Four
ofstreamobjects are opened and written without verifyinggood()/is_open(). If the directory is unwritable, the example silently prints paths to non-existent files. Consider at least a brief check after writing, consistent with the warning already emitted forcreate_directories.Example: minimal write-check helper
+ auto write_file = [](const string & path, const string & content) { + ofstream f(path); + f << content; + if (not f) + cout << " warning: failed to write " << path << '\n'; + }; + - { - ofstream fj(json_path); - fj << json; - } - { - ofstream fd(dot_path); - fd << dot; - } - { - ofstream fg(graphml_path); - fg << graphml; - } - { - ofstream fx(gexf_path); - fx << gexf; - } + write_file(json_path, json); + write_file(dot_path, dot); + write_file(graphml_path, graphml); + write_file(gexf_path, gexf);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/planarity_test_example.cc` around lines 313 - 328, The four file writes using ofstream (fj, fd, fg, fx) open and write to json_path, dot_path, graphml_path and gexf_path but do not verify success; add a short post-write check (e.g., ensure stream.is_open() and stream.good() or check after flush/close) and emit an error/warning if the write failed (consistent with the existing create_directories warning). Implement this by adding a small helper (e.g., write_file_or_warn) or inline checks after each write that report the path and failure reason if the stream is not good, referencing the existing variable names fj/fd/fg/fx and the path variables json_path/dot_path/graphml_path/gexf_path so failures are not silently ignored.Tests/geom_algorithms_test_common.h (2)
94-94:sort3andpolygon_signed_areaare missing[[maybe_unused]], inconsistent with every other helper in this namespace.Both are only reachable from
[[maybe_unused]]-annotated callers (canonical_triangles/polygon_area/is_ccw). A translation unit that includes this header but never invokes those callers may emit-Wunused-functionwarnings for these two.♻️ Proposed fix
- void sort3(size_t & a, size_t & b, size_t & c) + [[maybe_unused]] void sort3(size_t & a, size_t & b, size_t & c)- Geom_Number polygon_signed_area(const Polygon & poly) + [[maybe_unused]] Geom_Number polygon_signed_area(const Polygon & poly)Also applies to: 190-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/geom_algorithms_test_common.h` at line 94, Add the [[maybe_unused]] attribute to the helper functions sort3 and polygon_signed_area so they match the other helpers and won't trigger -Wunused-function when their only callers (canonical_triangles, polygon_area, is_ccw) are unused; update the function declarations/definitions for sort3 and polygon_signed_area to include [[maybe_unused]] in their signature.
35-35: Preferah_domain_error()overthrow std::domain_error; remove<stdexcept>.
throw std::domain_error(...)at line 176 is the only reason<stdexcept>is included. The rest of the codebase uses Aleph's own unconditional error macros; this should be consistent.♻️ Proposed change
-#include <stdexcept>- if (den == 0) - throw std::domain_error("circumcenter_of: collinear triangle (zero area)"); + if (den == 0) + ah_domain_error("circumcenter_of: collinear triangle (zero area)");Based on learnings: "Use unconditional Aleph error macros:
ah_domain_error(),ah_runtime_error(),ah_range_error(),ah_invalid_argument()".Also applies to: 175-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/geom_algorithms_test_common.h` at line 35, Replace the unconditional throw of std::domain_error with the project's macro ah_domain_error() and remove the now-unused `#include` <stdexcept>; specifically, locate the throw std::domain_error(...) usage (around the check at/near lines 175-176) and change it to call ah_domain_error(...) with the same message, and then delete the <stdexcept> include since it is no longer needed.Examples/blossom_example.cc (2)
86-98: Doxygen comment placed betweentemplateand function signature — move it before thetemplateline.Doxygen typically expects the documentation comment immediately before the full declaration (including the
templatekeyword). Placing it betweentemplate <class GT>and the function name can cause Doxygen to fail to associate the comment with the function. This pattern repeats at lines 113, 251, and 266.Proposed fix (example for build_graph)
- template <class GT> - /** - * `@brief` Constructs a graph of type `GT` that represents the given scenario. + /** + * `@brief` Constructs a graph of type `GT` that represents the given scenario. ... - */ + */ + template <class GT> GT build_graph(const Scenario & s)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/blossom_example.cc` around lines 86 - 98, Move the Doxygen block so it sits immediately before the entire templated declaration instead of between the template header and the function name: for example, place the comment now above the line "template <class GT>" that precedes GT build_graph(const Scenario & s). Do the same for the other templated functions that currently have their Doxygen comments inserted between "template <...>" and the function signature (i.e., relocate each comment to directly precede its corresponding "template <...>" line so Doxygen associates the comment with the full declaration).
56-56:std::setcould be replaced withDynSetTreeper Aleph container guidelines.
std::set<pair<size_t, size_t>>is used inwrite_tikz(line 189). The coding guidelines require Aleph containers when an equivalent exists. As this is an example file, it's a minor deviation.As per coding guidelines: "MUST use Aleph containers when an equivalent exists. Standard library containers are ONLY permitted when no Aleph equivalent is available." and "Use
DynSetTree<T>instead ofstd::set<T>".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/blossom_example.cc` at line 56, Replace the std::set<pair<size_t,size_t>> used in write_tikz with Aleph's DynSetTree<pair<size_t,size_t>>: change the `#include` for <set> to the Aleph DynSetTree header, update the container type in write_tikz and any variable declarations, and adjust calls that manipulate the set (insert, erase, find/contains, iteration) to use the DynSetTree API (same semantics but use DynSetTree methods/iterators). Ensure all references to the old std::set symbol are renamed to DynSetTree to satisfy the Aleph container guideline.Tests/ah-functional.cc (2)
1631-1645: Test name overstates what is verified — rename to match actual assertion.The test only checks
hash_calls > 0(i.e., that the custom hash functor is invoked at all). It does not verify thatDynSetHashis used internally; the namecustom_hash_functor_is_used_when_dynsethash_is_availableimplies a structural implementation guarantee that the assertion doesn't provide.Consider renaming to something like
custom_hash_functor_is_invokedto avoid overstating the invariant being tested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ah-functional.cc` around lines 1631 - 1645, The test currently named custom_hash_functor_is_used_when_dynsethash_is_available overstates its check; rename the TEST second parameter to something like custom_hash_functor_is_invoked (so the macro becomes TEST(all_unique_overload, custom_hash_functor_is_invoked)) and update any references/comments to match, since the body only asserts that the provided hash functor was invoked (hash_calls > 0) and does not verify use of DynSetHash or other internal implementation details.
1647-1661:empty_containerandsingle_elementtests don't cover the hash+eq overload.Both tests exercise only
all_unique(container, eq)andall_unique(container). The three-argument overloadall_unique(container, hash, eq)is not tested with empty or single-element containers, which are the most common edge-case failure points for hash-based implementations.➕ Suggested additions
TEST(all_unique_overload, empty_container) { DynList<int> empty; EXPECT_TRUE(all_unique(empty, std::equal_to<int>())); EXPECT_TRUE(all_unique(empty)); + // Hash+eq overload + EXPECT_TRUE(all_unique(empty, CaseInsensitiveStringHash(), CaseInsensitiveStringEqual())); } TEST(all_unique_overload, single_element) { auto single = build_dynlist<int>(42); EXPECT_TRUE(all_unique(single, std::equal_to<int>())); EXPECT_TRUE(all_unique(single)); + // Hash+eq overload + auto single_str = build_dynlist<string>("only"); + EXPECT_TRUE(all_unique(single_str, CaseInsensitiveStringHash(), CaseInsensitiveStringEqual())); }Based on learnings: "Tests must cover edge cases: empty inputs, single element, large inputs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ah-functional.cc` around lines 1647 - 1661, Add tests that exercise the three-argument overload all_unique(container, hash, eq) for the empty and single-element edge cases: update the tests named all_unique_overload::empty_container and all_unique_overload::single_element (which currently call all_unique(empty, std::equal_to<int>()) and all_unique(single, std::equal_to<int>())) to also call the hash+eq overload (e.g., pass a std::hash<int> or equivalent hash functor along with std::equal_to<int>), ensuring both EXPECT_TRUE assertions include the three-argument form using the same DynList<int> instances built with build_dynlist<int>().Examples/lca_example.cc (3)
75-78: Preferstd::arrayover C-style arrays.Both
NODE_LABELS(Line 75) andresults(Line 273) use C-style arrays of known compile-time size. As per coding guidelines,std::arrayshould be used instead.♻️ Proposed refactors
- static const char * const NODE_LABELS[NODES] = { + static constexpr std::array<const char *, NODES> NODE_LABELS = { "CEO", "CTO", "CFO", "Platform", "Data", "Accounting", "Legal", "SRE", "ML" - }; + };- const Backend_Result results[] = {list_result, sgraph_result, agraph_result}; + const std::array<Backend_Result, 3> results = {list_result, sgraph_result, agraph_result};As per coding guidelines: "Use
std::arrayinstead of C-style arrays when size is known at compile-time."Also applies to: 273-273
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/lca_example.cc` around lines 75 - 78, Replace the C-style arrays with std::array: include <array> and change NODE_LABELS to std::array<const char*, NODES> NODE_LABELS = { ... } and likewise change the results C-style array (referenced at symbol results) to an appropriate std::array<T, SIZE> with the same element type and compile-time size; update any code that takes the raw pointer/decays (e.g., passing to functions expecting T* or using sizeof tricks) to use results.data() or results.size() as needed. Ensure the element type matches (const char* or the actual type used for results) and keep the original initializer values.
109-123: Doxygen comment should precede thetemplatedeclaration, not follow it.The
/**@brief... */block at Line 110 comes aftertemplate <class GT>(Line 109). Doxygen conventionally expects the documentation comment to immediately precede the full declaration, including the template preamble, to ensure correct association.♻️ Proposed fix
- template <class GT> - /** - * `@brief` Builds a small rooted tree ... - * ... - */ - Backend_Result run_backend(const char * backend_name) + /** + * `@brief` Builds a small rooted tree ... + * ... + */ + template <class GT> + Backend_Result run_backend(const char * backend_name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/lca_example.cc` around lines 109 - 123, The Doxygen comment block is placed after the template declaration so it won't associate with the full template function; move the entire comment (the /** ... */ block) so it immediately precedes the template declaration for run_backend (i.e., place the comment above "template <class GT>" so the documentation attaches to the templated function Backend_Result run_backend(const char * backend_name)); ensure the comment still references GT and backend_name as before.
249-291:main()always returns0regardless of validation failures — CI cannot detect regressions.All the correctness flags (
deterministic_ok,engines_agree,checksum_equal,cross_backend_checksum_ok) are printed but never reflected in the exit code. A mis-implemented LCA engine would produce a silent pass in any automated pipeline.♻️ Proposed fix
+ const bool all_ok = cross_backend_checksum_ok and + std::ranges::all_of(results, [](const auto & r) { + return r.deterministic_ok and r.engines_agree and r.checksum_equal; + }); + + if (not all_ok) + { + std::cerr << "FAILURE: one or more validation checks failed.\n"; + return 1; + } + return 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/lca_example.cc` around lines 249 - 291, main() currently always returns 0 even when validation fails; change it to return non-zero on any failed check by evaluating the Backend_Result flags and cross_backend_checksum_ok: after computing list_result, sgraph_result, agraph_result and cross_backend_checksum_ok, compute a boolean failure = !list_result.deterministic_ok || !list_result.engines_agree || !list_result.checksum_equal || !sgraph_result.deterministic_ok || !sgraph_result.engines_agree || !sgraph_result.checksum_equal || !agraph_result.deterministic_ok || !agraph_result.engines_agree || !agraph_result.checksum_equal || !cross_backend_checksum_ok (or equivalent aggregated check over the results array), and return failure ? 1 : 0 from main() so CI fails when any correctness flag (deterministic_ok, engines_agree, checksum_equal, cross_backend_checksum_ok) is false.Examples/min_cost_matching_example.cc (1)
314-324: Same Doxygen misplacement as the weighted example — comment floats between}andmain().The
} /**on Line 314 closes the anonymous namespace and starts a Doxygen block that Doxygen won't associate withmain(). Move it directly aboveint main().Proposed fix
-} /** - * `@brief` Runs example demonstrations for minimum-cost matching ... - * `@return` int 0 on success. - */ - +} // namespace +/** + * `@brief` Runs example demonstrations for minimum-cost matching and perfect matching. + * `@return` int 0 on success. + */ int main()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/min_cost_matching_example.cc` around lines 314 - 324, The Doxygen block is misplaced after the anonymous-namespace closing brace; move the comment so it immediately precedes the definition of int main() (i.e., place the /** ... */ Doxygen comment directly above the main() function declaration) instead of sitting on the same line as the closing brace of the anonymous namespace to ensure Doxygen associates the documentation with main().Examples/weighted_blossom_example.cc (2)
363-373: Doxygen block formain()is misplaced between namespace brace and function.The
} /**on Line 363 closes the anonymous namespace and immediately starts a Doxygen block. This block is syntactically floating — Doxygen won't attach it tomain(). Move it directly aboveint main().Proposed fix
-} /** - * `@brief` Runs example scenarios demonstrating maximum-weight matching ... - * `@return` int Exit status code; `0` on successful completion. - */ - +} // namespace +/** + * `@brief` Runs example scenarios demonstrating maximum-weight matching and writes TikZ visualizations. + * + * `@return` int Exit status code; `0` on successful completion. + */ int main()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/weighted_blossom_example.cc` around lines 363 - 373, The Doxygen comment block currently sits after the anonymous-namespace closing brace (`}`) and therefore isn't attached to `int main()`; move the entire Doxygen block (the /** ... */ describing main and its return) so it immediately precedes the `int main()` definition (not after the namespace `}`), ensuring the block is directly above the `main()` signature so Doxygen associates the documentation with the `main()` function.
100-126: Doxygen comment placement: should precedetemplateline.The
template <class GT>declaration on Line 100 precedes the Doxygen block (Lines 101–112), meaning Doxygen will associate the comment with the template parameter list rather than the function. The same pattern repeats at Lines 129/284. Move the/** ... */block above thetemplateline.Proposed fix
+ /** + * `@brief` Construct a graph of type GT from a Scenario. + * ... + */ template <class GT> - /** - * `@brief` Construct a graph of type GT from a Scenario. - * ... - */ GT build_graph(const Scenario & s)Apply the same reorder to
solve_case(Line 129) andprint_backend(Line 284).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/weighted_blossom_example.cc` around lines 100 - 126, The Doxygen comment blocks are placed after the template declarations so they attach to the template parameter list instead of the function; move each /** ... */ block so it immediately precedes the corresponding template line for the functions build_graph, solve_case, and print_backend so the comment is associated with the function template (ensure the comment sits above "template <...>" for each of those symbols).Examples/min_mean_cycle_example.cc (2)
147-162:} /**mixes namespace closing brace andmain's Doxygen block on one line.Minor readability nit — add a blank line (and optionally an explicit end-of-namespace comment) between the two.
♻️ Suggested split
-} /** - * `@brief` Example program demonstrating Karp's minimum mean cycle algorithm on Aleph digraphs. +} // end anonymous namespace + +/** + * `@brief` Example program demonstrating Karp's minimum mean cycle algorithm on Aleph digraphs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/min_mean_cycle_example.cc` around lines 147 - 162, The closing brace and the Doxygen block for main are mashed together ("} /**") which hurts readability; split them so the namespace/class closing brace stands alone on its own line, add a blank line before the Doxygen comment that documents main, and optionally append an explicit end-of-namespace comment (e.g., "} // namespace <name>") to the brace; update the area around the end of the namespace and the start of int main() (symbols: the closing brace before the Doxygen block and the function main) accordingly.
58-72:Hide_Arcname is semantically inverted; struct lacks@brief.
operator()returnstruewhenarc != blocked— i.e., for arcs to include/keep, not arcs to hide. Readers expecting a "should-hide?" predicate will find the logic reversed. A name likeArc_VisibleorNot_Blockedbetter matches the return-value convention. Also, the struct itself has no Doxygen@brief; only itsoperator()is documented.♻️ Suggested rename and struct-level doc
+ /** + * `@brief` Arc-filter predicate: returns true for arcs that should be + * included in the traversal (i.e., arcs that are not blocked). + */ - struct Hide_Arc + struct Arc_Visible { Arc * blocked = nullptr; /** - * `@brief` Predicate that checks whether an arc is not the blocked arc. + * `@brief` Returns true if `@p` arc is not the blocked arc. * * `@param` arc Pointer to the arc to test; may be nullptr. - * `@return` `true` if `arc` is not equal to the predicate's `blocked` arc, `false` otherwise. + * `@return` `true` if `@p` arc should be included (is not the blocked arc). */ bool operator()(Arc * arc) const noexcept { return arc != blocked; } };and update the call site:
- karp_minimum_mean_cycle<Graph, Dft_Dist<Graph>, Hide_Arc>( - g, Dft_Dist<Graph>(), Hide_Arc{blocked}); + karp_minimum_mean_cycle<Graph, Dft_Dist<Graph>, Arc_Visible>( + g, Dft_Dist<Graph>(), Arc_Visible{blocked});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/min_mean_cycle_example.cc` around lines 58 - 72, The predicate struct Hide_Arc is semantically inverted: operator()(Arc * arc) returns true when arc != blocked (i.e., arc is visible), so rename the struct to a name that reflects that behavior (e.g., Arc_Visible or Not_Blocked) and add a Doxygen `@brief` on the struct itself describing that it tests whether an arc is not the blocked arc; then update all call sites that construct or refer to Hide_Arc to use the new name (and keep operator() implementation as-is) so the predicate name matches its return-value convention.Examples/k_shortest_paths_example.cc (2)
161-196: Preferstd::formatoverstd::coutstream expressions per coding guidelines.The output section uses raw
std::cout <<with string concatenation, while the guidelines specify "std::formatfor type-safe string formatting (prefer over printf/streams)". Consider replacing the stream expressions withstd::format/std::printcalls for consistency with the project style.♻️ Example replacement for lines 142–145
- cout << " #" << rank - << " cost=" << item.total_cost - << " path=" << path_to_string(item.path) - << '\n'; + cout << std::format(" #{:} cost={:} path={:}\n", + rank, item.total_cost, path_to_string(item.path));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/k_shortest_paths_example.cc` around lines 161 - 196, Replace the raw std::cout stream expressions in main() (the lines printing header and "source=..., target=..., k=...") with type-safe formatting calls using std::format or std::print; specifically, construct a single formatted string for the header and a single formatted string for the source/target/k line (using scenario.source, scenario.target, scenario.k) and call std::print or output the formatted std::string instead of chaining operator<< on std::cout, keeping print_results("Yen (loopless/simple paths)", yen) and print_results("Eppstein-style API (general, may include cycles)", epp) unchanged.
124-141:print_resultstemplate uses a hardcoded concrete type — either de-templatize or use the list's element type.
template <class Result_List>implies the function is generic over the container, but line 141 binds directly toResult_Item(a concrete alias forK_Shortest_Path_Item<Graph, long long>). Any instantiation with a different element type silently fails to compile at that binding, and the template parameter adds no real generality.Additionally, per coding guidelines, template parameters should be constrained with concepts rather than left unconstrained.
♻️ Option A — Remove the template (both callers pass the same list type)
- template <class Result_List> void print_results(const string & title, - const Result_List & results) + const DynList<Result_Item> & results)♻️ Option B — Make the element type a second template parameter and add a concept
- template <class Result_List> + template <typename Item, typename Result_List> + requires requires (Result_List l) { { l.is_empty() } -> std::convertible_to<bool>; } void print_results(const string & title, const Result_List & results) { ... - const Result_Item & item = it.get_curr(); + const Item & item = it.get_curr();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/k_shortest_paths_example.cc` around lines 124 - 141, print_results is declared as template<class Result_List> but binds a concrete type via "const Result_Item & item = it.get_curr()", which breaks generality; either remove the template and use the concrete list type (de-templatize) since both callers use the same list, or make the element type explicit and constrain it with a concept: change the signature to accept the list's element type (e.g., template<class Result_List, class Result_Item> or template<class Result_List> with typename Element = typename Result_List::value_type) and add a concept requiring an Iterator with has_curr()/next_ne()/get_curr() and an element exposing total_cost and path; replace the hardcoded `Result_Item` binding with the deduced element type (or typename Result_List::value_type) and add the concept in the template parameter list so callers and compilation are correct (update print_results signature and any callers accordingly).Examples/heavy_light_decomposition_example.cc (3)
160-162: C-style arrays — preferstd::arrayper project guidelines
labels, the anonymousqueriesstruct array,ordered_nodes(once added) — all have compile-time-known sizes.♻️ Example change for the labels array
- const char * labels[] = { - "Central", "North", "West", "South", "N-A", - "N-B", "S-A", "S-A1", "S-A2"}; + constexpr std::array<const char *, 9> labels = { + "Central", "North", "West", "South", "N-A", + "N-B", "S-A", "S-A1", "S-A2"};As per coding guidelines: "Use
std::arrayinstead of C-style arrays when size is known at compile-time."Also applies to: 178-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/heavy_light_decomposition_example.cc` around lines 160 - 162, Replace C-style arrays with std::array for all compile-time sized collections: change the const char* labels[] to std::array<const char*, N> (use the actual element count), convert the anonymous queries struct array to std::array<QueryType, M> (use the concrete struct type and element count), and make ordered_nodes a std::array<NodeType, K> when its size is known; include <array> and update initializers to use brace-enclosed lists or CTAD as appropriate so construction and usages (e.g., indexing, size()) keep working with the existing functions and code paths referencing labels, the queries array, and ordered_nodes.
45-49: Non-standard whitespace in preprocessor directivesEvery other file in this PR (including
hld_example.cc) uses#includewithout a space. The space is technically tolerated by GCC/Clang but is non-idiomatic and inconsistent.✂️ Proposed fix
-# include <Tree_Decomposition.H> -# include <tpl_graph.H> - -# include <cassert> -# include <cstdio> +#include <Tree_Decomposition.H> +#include <tpl_graph.H> + +#include <cassert> +#include <cstdio>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/heavy_light_decomposition_example.cc` around lines 45 - 49, The preprocessor directives in Examples/heavy_light_decomposition_example.cc use a non-idiomatic space after the '#' (e.g. "# include <Tree_Decomposition.H>", "# include <tpl_graph.H>", "# include <cassert>", "# include <cstdio>"); update each to the standard form ("#include <...>") so they match the style used elsewhere (like hld_example.cc) and avoid inconsistent formatting across the PR.
171-242: Preferstd::format/std::printoverstd::printfThe coding guidelines specify: "Use
std::formatfor type-safe string formatting (prefer over printf/streams)". Allstd::printfcalls in this file could be replaced withstd::print(C++23) orstd::cout << std::format(...)(C++20).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/heavy_light_decomposition_example.cc` around lines 171 - 242, Replace all uses of std::printf in Examples/heavy_light_decomposition_example.cc with type-safe C++ formatting/printing: use std::print (C++23) or std::cout << std::format(...) (C++20). Update the include directives (include <format> and/or <iostream>) and convert each format string to std::format-style placeholders (e.g. "id={} {} pos={}" etc.) for the blocks that print node mapping, "Path maintenance cost queries", subtree totals, budget changes, per-segment messages inside the hld.for_each_path_segment lambda, and the final messages; also replace the numeric format/width assumptions from "%zu"/"%d"/"%-18s"/"%-8s" to appropriate std::format formatting (e.g. {:zu} -> {}, align/width with {:<18}, etc.), and ensure the build uses the required C++ standard when choosing std::print vs std::format.Examples/hld_example.cc (2)
181-182:constshould beconstexprfor consistency with Scenario 1Scenario 1 (line 106) correctly uses
constexpr Query queries[]. Scenario 2 usesconstinstead, without reason.♻️ Proposed fix
- const Query queries[] = {{6, 5}, {4, 3}, {6, 3}, {1, 2}}; + constexpr Query queries[] = {{6, 5}, {4, 3}, {6, 3}, {1, 2}};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/hld_example.cc` around lines 181 - 182, The queries array in Scenario 2 is declared with const but should be constexpr for consistency with Scenario 1; update the declaration of the array named queries (and any related immutable test data in the same scope) from const Query queries[] to constexpr Query queries[] so the compiler can evaluate it at compile time, matching the existing constexpr usage in the other scenario and ensuring identical semantics for Query (struct Query { size_t u, v; }).
98-100: C-style arrays — preferstd::arrayper project guidelines
names,clearance,bandwidth, andtaxesarrays all have compile-time-known sizes across all three scenarios.♻️ Example change for Scenario 1
- const char * names[] = {"CEO", "CTO", "CFO", "Eng", "Data", - "Acct", "Legal", "ML"}; - const int clearance[] = {5, 4, 3, 2, 3, 1, 2, 3}; + constexpr std::array<const char *, 8> names = {"CEO", "CTO", "CFO", "Eng", "Data", + "Acct", "Legal", "ML"}; + constexpr std::array<int, 8> clearance = {5, 4, 3, 2, 3, 1, 2, 3};As per coding guidelines: "Use
std::arrayinstead of C-style arrays when size is known at compile-time."Also applies to: 175-176, 244-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/hld_example.cc` around lines 98 - 100, Replace the C-style arrays (names, clearance, bandwidth, taxes) with std::array instances of the appropriate compile-time size and element type, add `#include` <array> if missing, keep them const (or constexpr) as before, and update any code that treats them as pointers (e.g., sizeof/iteration) to use .size() and range-for or indexed access so the semantics remain identical; specifically change the declarations for names and clearance (and the other scenario occurrences) to std::array<const char*, N> and std::array<int, N> respectively, preserving the values and constness.Examples/centroid_decomposition_example.cc (3)
134-135: Defensively initializelabel_by_idandnode_by_idbefore the mapping loop.
Array<T>::create(n)likely leaves elements uninitialised. For an 11-node graph all 11 IDs are covered, so this is not a live bug today. However, ifnever exceedsNUM_NODES(e.g., when the graph backend assigns synthetic sentinel IDs), any unset slot accessed in aprintforhldcall becomes UB.♻️ Proposed defensive initialization
auto label_by_id = Array<const char *>::create(n); auto node_by_id = Array<Node *>::create(n); + for (size_t i = 0; i < n; ++i) + { + label_by_id(i) = "(unknown)"; + node_by_id(i) = nullptr; + } + for (int i = 0; i < NUM_NODES; ++i)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/centroid_decomposition_example.cc` around lines 134 - 135, The arrays label_by_id and node_by_id are left uninitialized by Array<T>::create(n), so before the mapping loop set every slot to safe defaults (e.g., label_by_id[i] = "" or a sentinel const char* and node_by_id[i] = nullptr) to avoid UB when later printed or passed to hld; update the code around the Array<const char *>::create and Array<Node *>::create usage (labels and node_by_id) to initialize all n entries (via a fill/for loop or an Array creation helper) so every index is valid even if n grows.
126-132: Preferstd::arrayover C-style arrays and derive size from the array itself.
nodes_arrandvillage_labelare C-style arrays with compile-time-known sizes; the guidelines requirestd::array. Additionally,constexpr int NUM_NODES = 11duplicates the array length as a magic number —std::size(nodes_arr)(orstd::ssize) would remove the redundancy and prevent divergence.♻️ Proposed refactor
- Node * nodes_arr[] = { n0, n1, n2, n3, n4, n5, n6, n7, n8, n9, n10 }; - const char * village_label[] = { - "Village-0", "Village-1", "Village-2", "Village-3", "Village-4", - "Village-5", "Village-6", "Village-7", "Village-8", "Village-9", - "Village-10" - }; - constexpr int NUM_NODES = 11; + const std::array<Node *, 11> nodes_arr = { n0, n1, n2, n3, n4, n5, n6, n7, n8, n9, n10 }; + const std::array<const char *, 11> village_label = { + "Village-0", "Village-1", "Village-2", "Village-3", "Village-4", + "Village-5", "Village-6", "Village-7", "Village-8", "Village-9", + "Village-10" + }; + constexpr size_t NUM_NODES = std::size(nodes_arr);As per coding guidelines: "Use
std::arrayinstead of C-style arrays when size is known at compile-time."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/centroid_decomposition_example.cc` around lines 126 - 132, Replace the C-style arrays nodes_arr and village_label with std::array (e.g., std::array<Node*, N> and std::array<const char*, N>) and remove the hard-coded constexpr int NUM_NODES; instead derive the count from the array (e.g., using nodes_arr.size() or std::size(nodes_arr)) wherever NUM_NODES was used; update any code that indexes or iterates (references to nodes_arr, village_label, and NUM_NODES) to use the std::array methods and the computed size.
195-233: Replacestd::printfcalls withstd::format/std::printper project guidelines.All diagnostic output uses
std::printf. The coding guidelines preferstd::format(C++20) for type-safe string formatting. Even in example files, consistency with the rest of the codebase is valuable.Example replacement for the centroid-root line:
♻️ Example replacement
- std::printf("=== Emergency Response Network (Centroid Decomposition) ===\n\n"); - std::printf("Centroid root: %s (id=%zu)\n\n", - label_by_id(cd.centroid_root_id()), - cd.centroid_root_id()); + std::print("=== Emergency Response Network (Centroid Decomposition) ===\n\n"); + std::print("Centroid root: {} (id={})\n\n", + label_by_id(cd.centroid_root_id()), + cd.centroid_root_id());(
std::printis C++23; for strict C++20 usestd::cout << std::format(...). Add#include <print>or#include <format>accordingly.)As per coding guidelines: "Use
std::formatfor type-safe string formatting (prefer over printf/streams)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/centroid_decomposition_example.cc` around lines 195 - 233, The example uses C-style std::printf calls; update these to the project-preferred C++ formatted output (use std::print if the project targets C++23, or std::cout << std::format(...) for C++20). Replace each printf invocation around centroid output, ancestor loop, activation messages and nearest-result prints (references: cd, label_by_id, cd.for_each_centroid_ancestor_id, activate, query, brute_query, Node pointers n4,n5,n7,n10,n2,n6,n9,n10) with the chosen std::print or std::format/std::cout form and add the appropriate header (<print> or <format>) at the top of the file.
Refactored test examples to use DynArray and DynSetTree for consistency. Updated K_Shortest_Paths.H with detailed Doxygen documentation. Introduced Ruby scripts for CI batch visual diff and validation of planarity certificates, enhancing testing capabilities. Adjusted CI workflow to include Ruby and update visual golden diff for planarity.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Tests/k2tree_test.cc (1)
462-462:⚠️ Potential issue | 🟡 MinorTrivially-true assertion provides no test value
result.size()returnssize_t(unsigned), soEXPECT_GE(result.size(), 0)is always satisfied and catches nothing. Either remove it or replace it with a meaningful assertion, e.g. comparing the result count against a brute-force count over the inserted points.🛠️ Suggested fix
- // Just verify it doesn't crash - EXPECT_GE(result.size(), 0); + // Verify result count does not exceed total inserted points + EXPECT_LE(result.size(), static_cast<size_t>(1000));Or remove the assertion entirely and add a comment that the loop is a crash-safety smoke test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/k2tree_test.cc` at line 462, The EXPECT_GE(result.size(), 0) assertion is trivially true because result.size() is unsigned; replace or remove it: either compute an expected_count via a brute-force scan of the inserted points and assert EXPECT_EQ(result.size(), expected_count) (or EXPECT_GE/EXPECT_LE with known bounds), or remove the assertion and add a comment that the surrounding loop is a crash-safety smoke test; locate and update the EXPECT_GE(result.size(), 0) line and the surrounding test logic that produces result to implement the meaningful comparison.Tests/geom_algorithms_test_boolean_3d_serializer_aabb_misc.cc (1)
1604-1654:⚠️ Potential issue | 🟠 MajorTests ConvexPolygonOffsetCollinearConsecutiveVertices and ConvexPolygonOffsetLargeInwardOffset are active but expected to fail CI.
Both tests assert correct behavior while their inline comments claim the implementation produces wrong results:
- Test at line 1630 expects
offset_area < orig_area, but comment states impl producesarea > 100- Test at line 1652 expects
result.size() < 3, but comment states impl produces non-degenerate polygonThese tests must be either disabled or rewritten as regression guards:
Option A: Disable until fixed
-TEST_F(GeomAlgorithmsTest, ConvexPolygonOffsetCollinearConsecutiveVertices) +TEST_F(GeomAlgorithmsTest, DISABLED_ConvexPolygonOffsetCollinearConsecutiveVertices)-TEST_F(GeomAlgorithmsTest, ConvexPolygonOffsetLargeInwardOffset) +TEST_F(GeomAlgorithmsTest, DISABLED_ConvexPolygonOffsetLargeInwardOffset)Option B: Assert buggy behavior as regression guard
- EXPECT_LT(offset_area, orig_area) + EXPECT_GE(offset_area, orig_area) // TODO: collinear vertex handling bug- EXPECT_LT(result.size(), 3u) + EXPECT_GE(result.size(), 3u) // TODO: half-plane intersection bug with large offsets🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/geom_algorithms_test_boolean_3d_serializer_aabb_misc.cc` around lines 1604 - 1654, The two tests ConvexPolygonOffsetCollinearConsecutiveVertices and ConvexPolygonOffsetLargeInwardOffset assert correct behavior but are known to fail; either disable them by renaming the test cases to prefix DISABLED_ (e.g., DISABLED_ConvexPolygonOffsetCollinearConsecutiveVertices, DISABLED_ConvexPolygonOffsetLargeInwardOffset) so CI skips them, or convert them into regression-guard checks that assert the current buggy behavior (e.g., replace EXPECT_LT(offset_area, orig_area) with EXPECT_GT(offset_area, orig_area) in ConvexPolygonOffsetCollinearConsecutiveVertices and replace EXPECT_LT(result.size(), 3u) with EXPECT_GE(result.size(), 3u) in ConvexPolygonOffsetLargeInwardOffset) and add comments referencing the bug id; pick one approach consistently and keep test names (or comments) indicating that these are temporary until ConvexPolygonOffset is fixed.
🧹 Nitpick comments (12)
Examples/planarity_test_example.cc (1)
180-448:mainis ~270 lines with four inline demo blocks — consider extracting helpers for readability.Each demo block (planar sample, dense sample, K3,3, digraph) is self-contained and could be a named function (e.g.,
demo_planar_sample,demo_k33_certificate). This would reduce cognitive load and make it easier to add/remove demos. Low priority given this is an example file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/planarity_test_example.cc` around lines 180 - 448, main is long because it contains four large inline demo blocks; extract each block into its own helper function (e.g., demo_planar_sample(UGraph&, const Planarity_Test_Options&, const fs::path&), demo_dense_sample(...), demo_k33_certificate(const fs::path&, const Planarity_Test_Options&), demo_digraph_sample(DGraph&, const Planarity_Test_Options&)) and move the corresponding logic (graph construction, planarity_test calls, printing, certificate file writing and validation) into those functions, preserving use of symbols like Planarity_Test_Options, strict_lr_opts, planarity_test, nonplanar_certificate_to_json/dot/graphml/gexf, validate_nonplanar_certificate, planar_geometric_drawing, planar_dual_metadata; make each helper take only the inputs it needs (output_dir and opts where appropriate), return void (or a small result struct if needed), and replace the inline blocks in main with simple calls to these helpers to improve readability.Examples/hungarian_example.cc (3)
137-141:hashould beconstfor consistency withexample_basic_assignment
hais not modified after construction. Marking itconstaligns with the pattern inexample_basic_assignment(line 57) and satisfies "Mark all non-modifying member functions asconst" / "Useconstfor variables that don't change after initialization."♻️ Proposed fix
- Hungarian_Assignment<int> ha({ + const Hungarian_Assignment<int> ha({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/hungarian_example.cc` around lines 137 - 141, The local variable ha (constructed as Hungarian_Assignment<int> ha({...})) is not modified after construction—change its declaration to const Hungarian_Assignment<int> ha({...}) to match the pattern used in example_basic_assignment and to signal immutability; locate the Hungarian_Assignment<int> ha construction and prefix it with const.
104-107: Replace C-style 2D array withstd::array
int data[3][3]is a C-style array with a compile-time-known size.♻️ Proposed refactor
- int data[3][3] = {{10, 5, 13}, {3, 9, 18}, {10, 6, 12}}; - for (size_t i = 0; i < 3; ++i) - for (size_t j = 0; j < 3; ++j) - mat(i, j) = data[i][j]; + using Row3 = std::array<int, 3>; + constexpr std::array<Row3, 3> data = {{{10, 5, 13}, {3, 9, 18}, {10, 6, 12}}}; + for (size_t i = 0; i < data.size(); ++i) + for (size_t j = 0; j < data[i].size(); ++j) + mat(i, j) = data[i][j];As per coding guidelines: "Use
std::arrayinstead of C-style arrays when size is known at compile-time."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/hungarian_example.cc` around lines 104 - 107, Replace the C-style array 'data' with a std::array to follow guidelines: change the declaration of data to std::array<std::array<int, 3>, 3> data = {{{{10,5,13}},{{3,9,18}},{{10,6,12}}}}; (or equivalent nested std::array initializer), add `#include` <array> if missing, and keep the existing loops that assign mat(i, j) = data[i][j]; so only the type of 'data' and its initializer change while preserving its indexing and use in the mat population code.
57-80: Cost matrix is defined twice; preferstd::arrayover C-style arrayThe cost data appears identically at lines 58–62 (inside the constructor call) and again at lines 71–76 as a raw C-style
int[4][4]. A singleconstexpraggregate can serve both purposes, eliminating the duplication and the drift risk. Additionally, per coding guidelines,std::arrayshould replace C-style arrays when the size is known at compile time.♻️ Proposed refactor
- const Hungarian_Assignment<int> ha({ - {82, 83, 69, 92}, - {77, 37, 49, 92}, - {11, 69, 5, 86}, - { 8, 9, 98, 23} - }); - - cout << "Optimal total cost: " << ha.get_total_cost() << endl; - cout << "Assignments:" << endl; - for (auto [r, c] : ha.get_assignments()) - cout << " Worker " << r << " -> Task " << c << endl; - cout << endl; - - // Show individual costs - constexpr int costs[4][4] = { - {82, 83, 69, 92}, - {77, 37, 49, 92}, - {11, 69, 5, 86}, - { 8, 9, 98, 23} - }; - cout << "Detailed:" << endl; - for (auto [r, c] : ha.get_assignments()) - cout << " Worker " << r << " -> Task " << c - << " (cost " << costs[r][c] << ")" << endl; + using Row4 = std::array<int, 4>; + constexpr std::array<Row4, 4> costs = {{ + {82, 83, 69, 92}, + {77, 37, 49, 92}, + {11, 69, 5, 86}, + { 8, 9, 98, 23} + }}; + + const Hungarian_Assignment<int> ha(costs); // assuming the ctor accepts a range/array + + cout << "Optimal total cost: " << ha.get_total_cost() << endl; + cout << "Assignments:" << endl; + for (auto [r, c] : ha.get_assignments()) + cout << " Worker " << r << " -> Task " << c << endl; + cout << endl; + + cout << "Detailed:" << endl; + for (auto [r, c] : ha.get_assignments()) + cout << " Worker " << r << " -> Task " << c + << " (cost " << costs[r][c] << ")" << endl;As per coding guidelines: "Use
std::arrayinstead of C-style arrays when size is known at compile-time" and "Refactor duplicated code into reusable functions or templates."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/hungarian_example.cc` around lines 57 - 80, The cost matrix is duplicated as a literal and a C-style int[4][4]; define a single constexpr std::array<std::array<int,4>,4> (e.g. constexpr std::array<std::array<int,4>,4> cost = {{ {...}, {...}, {...}, {...} }}), use that same constexpr when constructing Hungarian_Assignment<int> (pass cost) and use cost[r][c] for the detailed output; replace the C-style costs array with this std::array and update references to ha, Hungarian_Assignment, and costs accordingly so there is a single compile-time source of truth.Examples/min_mean_cycle_example.cc (3)
144-144: Preferauto *over the explicitArc *type.
Arc * arc = it.get_curr();can useauto *to let the type be deduced, which is consistent with the project's C++20 style guidelines and removes the redundant explicit type that must be kept in sync with the iterator's value type.♻️ Proposed fix
- Arc * arc = it.get_curr(); + auto * arc = it.get_curr();As per coding guidelines: "Use
autofor type deduction."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/min_mean_cycle_example.cc` at line 144, Replace the explicit pointer declaration "Arc * arc = it.get_curr();" with type-deducing auto to follow project C++20 style: use "auto * arc = it.get_curr();" (i.e., change the declaration in Examples/min_mean_cycle_example.cc where Arc, arc and it.get_curr() are used) so the compiler deduces the pointer type from the iterator's get_curr() return type.
156-168: Namespace closing brace and Doxygen comment are fused on the same line.Line 156 reads
} /**, placing the anonymous namespace closing brace immediately before the@briefblock formain(). While valid C++, it is visually confusing and unusual. Separate them with a blank line for clarity.♻️ Proposed fix
-} /** +} + +/** * `@brief` Example program demonstrating Karp's minimum mean cycle algorithm on Aleph digraphs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/min_mean_cycle_example.cc` around lines 156 - 168, The anonymous namespace closing brace '}' is fused with the Doxygen comment for main (the line starting with "} /**"), which is confusing; edit the closing brace for the anonymous namespace (the lone '}') so it sits on its own line followed by a blank line, then keep the Doxygen block (/** ... `@brief` ... */) for main() starting on the next line; this affects the anonymous namespace closing token and the main() Doxygen comment near the end of Examples/min_mean_cycle_example.cc.
120-122: Preferah_fatal_error()over bareabort().
abort()bypasses the Aleph error-reporting infrastructure. The coding guidelines mandateah_fatal_error()for unrecoverable fatal conditions in all*.{H,cc}files.♻️ Proposed fix
+#include <ah-errors.H> cerr << "Sanity check failed: witness mean " << witness_mean << " != minimum mean " << r.minimum_mean << endl; - abort(); + ah_fatal_error("witness mean does not match reported minimum mean");As per coding guidelines: "Use
ah_fatal_error()macro for unrecoverable fatal errors."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/min_mean_cycle_example.cc` around lines 120 - 122, Replace the bare abort() call with the ah_fatal_error() macro so the fatal condition is reported through Aleph's error infrastructure; specifically, in the block that prints the sanity check using witness_mean and r.minimum_mean (the cerr << ... lines and abort()), call ah_fatal_error() with a descriptive message including witness_mean and r.minimum_mean instead of abort() so the error is handled by the project’s logging/diagnostics.Tests/k2tree_test.cc (1)
44-46: Replacestd::vectorandstd::setwith Aleph equivalents throughout
std::vector<Point>(Lines 174, 582, 613, 736) andstd::set<std::pair<double,double>>(Lines 662, 935, 943) violate the mandatory container policy. Replace withDynList<Point>andDynSetTree<std::pair<double,double>>respectively, and drop the<vector>/<set>includes in favour of the corresponding Aleph headers.As per coding guidelines: "MUST use Aleph containers when an equivalent exists. Use
DynList<T>instead ofstd::list<T>orstd::vector<T>. UseDynSetTree<T>instead ofstd::set<T>."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/k2tree_test.cc` around lines 44 - 46, Replace all uses of std::vector<Point> with Aleph's DynList<Point> (occurrences around the symbols in Tests/k2tree_test.cc at the noted spots: lines ~174, ~582, ~613, ~736) and replace std::set<std::pair<double,double>> with DynSetTree<std::pair<double,double>> (occurrences around lines ~662, ~935, ~943); remove the `#include` <vector> and `#include` <set> headers and add the corresponding Aleph container headers (e.g., the DynList and DynSetTree headers used in our codebase) so the file uses Aleph containers everywhere while leaving <cmath> intact. Ensure template parameters and any construction/iteration sites are updated to the DynList/DynSetTree APIs if needed.Tests/geom_algorithms_test_boolean_3d_serializer_aabb_misc.cc (1)
1261-1274: LGTM —.length()is the correct geometric metric here.Using
.size()on aSegmentreturns the number of vertices (a constant2), making the old assertions trivially equivalent to2 > 2→ always false, which would have caused these tests to silently always fail or assert nothing meaningful..length()returns the Euclidean length and is exactly what's needed to verify thatenlarge_src/enlarge_tgtextend the segment.🔧 Optional: prefer
EXPECT_GToverEXPECT_TRUE(a > b)for richer failure output- EXPECT_TRUE(s_copy.length() > s.length()); + EXPECT_GT(s_copy.length(), s.length()); - EXPECT_TRUE(s_copy2.length() > s.length()); + EXPECT_GT(s_copy2.length(), s.length());
EXPECT_GTprints both values on failure, which makes diagnosing regressions much easier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/geom_algorithms_test_boolean_3d_serializer_aabb_misc.cc` around lines 1261 - 1274, Replace the weak boolean assertions with richer failure output: in the test SegmentEnlargeDiagonal, keep using Segment::length() to compare lengths after enlarge_src/enlarge_tgt (on s_copy and s_copy2) but change EXPECT_TRUE(s_copy.length() > s.length()) and EXPECT_TRUE(s_copy2.length() > s.length()) to EXPECT_GT(s_copy.length(), s.length()) and EXPECT_GT(s_copy2.length(), s.length()) respectively so failures print both values and aid debugging; target the assertions involving s_copy, s_copy2, Segment::length(), enlarge_src, and enlarge_tgt.K_Shortest_Paths.H (3)
251-255:Cost_Typetemplate parameter is unused inPath_Snapshot.The struct body contains only
Array<Node*>andArray<Arc*>;Cost_Typeis never referenced inside it.♻️ Proposed simplification
- template <class GT, typename Cost_Type> - struct Path_Snapshot - { - Array<typename GT::Node *> nodes; - Array<typename GT::Arc *> arcs; - }; + template <class GT> + struct Path_Snapshot + { + Array<typename GT::Node *> nodes; + Array<typename GT::Arc *> arcs; + };All call sites (
make_path_snapshot<GT, Cost_Type>,same_prefix_nodes<GT, Cost_Type>, etc.) would need theCost_Typeargument dropped from thePath_Snapshottemplate arguments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@K_Shortest_Paths.H` around lines 251 - 255, Path_Snapshot's template parameter Cost_Type is unused; change the declaration of struct Path_Snapshot to remove the Cost_Type template parameter (so it only depends on GT) and then update all call sites that reference Path_Snapshot with two template args (e.g., make_path_snapshot<GT, Cost_Type>, same_prefix_nodes<GT, Cost_Type>, and any other instantiations) to pass only GT (e.g., make_path_snapshot<GT>, same_prefix_nodes<GT>), adjusting forward declarations and using statements accordingly to ensure builds still find Path_Snapshot<GT>.
1007-1016: O(k) linear scan for the minimum candidate contradicts the documented O(k log k) complexity.Both
yen_k_shortest_paths(lines 1007–1016) andeppstein_k_shortest_paths(lines 1119–1122) use the same O(k) scan-and-swap-last idiom to extract the cheapest candidate on every iteration. Over k iterations this gives O(k²) extraction, whereas the documented complexity foreppstein_k_shortest_pathsisO(E + V log V + k log k)— the latter requires a heap.The main advantage of Eppstein's algorithm is that it achieves O(m + n log n + k log k) complexity, guaranteeing paths are produced in sorted order by weight. The current linear scan negates this.
Replace
candidateswith an Aleph min-heap (as required by the coding guidelines) so extraction is O(log k):♻️ Proposed sketch
- Array<State> candidates; - candidates.reserve(k * 3 + 1); + // Use a min-heap keyed on total_cost for O(log k) extraction. + // Replace Array + linear scan with an Aleph heap implementation. ... - size_t best_idx = 0; - for (size_t i = 1; i < candidates.size(); ++i) - if (candidates[i].total_cost < candidates[best_idx].total_cost) - best_idx = i; - - State best = candidates[best_idx]; - if (best_idx + 1 < candidates.size()) - candidates[best_idx] = candidates.get_last(); - const auto removed_candidate = candidates.remove_last(); - (void) removed_candidate; + State best = /* heap.top() */; + /* heap.pop(); */Also fix the documented
@complexityforeppstein_k_shortest_pathsto reflect the actual O(k²) extraction until this is resolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@K_Shortest_Paths.H` around lines 1007 - 1016, The current linear scan in yen_k_shortest_paths and eppstein_k_shortest_paths that finds the best candidate by iterating over candidates (State vector and indices best_idx/get_last/remove_last) makes extractions O(k) and yields O(k²) overall; replace the candidates container with an Aleph min-heap (use the provided heap API for push and pop/min operations) so you push new State entries into the heap and extract the minimum via heap_pop (O(log k)) instead of the scan+swap_last/remove_last pattern, adjust any code that relied on get_last/remove_last to use heap_pop/heap_top, and update the `@complexity` doc for eppstein_k_shortest_paths if you cannot complete the heap refactor immediately.
800-871:contains_pathdeduplication is O(|container| × path_length) per call, giving O(k² × path_length) total.Both
generate_general_deviation_candidates(lines 862–864) and the Yen inner loop (lines 999–1001) callcontains_pathwhich walks the fullaccepted/candidatesarrays and does a node+arc sequence comparison for each entry. For the Eppstein case the candidates array can grow to O(k × degree); for Yen's it is O(k). Either way the total cost grows quadratically with k.A hash of the arc-pointer sequence (inserted into an
OLhashTableorDynSetTreeof path signatures) would reduce duplicate detection to O(path_length) amortized:♻️ Suggested approach
// Hash path identity by XOR-combining arc pointer values. // Keep a DynSetTree<size_t> seen_hashes alongside the candidates array. // Before appending, compute hash(candidate.arcs) and check seen_hashes. // This drops the O(k) membership scan to O(log k) or O(1).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@K_Shortest_Paths.H` around lines 800 - 871, The deduplication via contains_path in generate_general_deviation_candidates (and the Yen inner loop that also calls contains_path) is O(k) per candidate; replace it with a path-signature set: compute a compact signature for a Path_State by hashing its arc-pointer sequence (e.g., XOR/combine reinterpret_cast<size_t>(arc) across candidate.arcs and include arc count or another small discriminator), maintain a DynSetTree<size_t> or OLhashTable<size_t> called seen_hashes alongside candidates/accepted, check seen_hashes before appending, and insert the signature when you append a candidate; update both generate_general_deviation_candidates (where candidates.append(candidate) is called) and the Yen candidate insertion site to use the signature-set check instead of contains_path to reduce dedup complexity to amortized O(path_length).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/planarity_gephi_nightly.yml:
- Around line 169-176: The script uses re.search (call site: re.search) but the
top import block (which currently lists json, os, pathlib, shutil, stat,
tarfile, urllib.request, zipfile) is missing the re module, causing a NameError
on Windows runners; fix by adding import re to that import list so re.search is
defined and asset scoring won't crash.
- Around line 230-236: The OS check currently uses elif runner_os == "MacOS"
which misses GitHub's "macOS" casing; update the condition to normalize
runner_os (e.g., compare runner_os.lower() to "macos" or otherwise accept
"macOS") so the macOS branch that adjusts score (the block referencing
runner_os, runner_arch, and lower) executes correctly and macOS assets reach the
intended ≥80 threshold.
In `@Blossom_Weighted.H`:
- Around line 356-403: Add full Doxygen `@param`, `@return`, and `@throws` tags to the
public wrapper blossom_maximum_weight_matching and the functor
Compute_Maximum_Weight_General_Matching (including its constructor and
operator()) — document parameters g, matching, weight, sa, and max_cardinality,
describe the return type Blossom_Weighted_Result<long long>, and list possible
exceptions/errors the functions can raise (e.g., invalid input, allocation or
algorithm failures) so the public API has `@brief`, `@param`, `@return`, and `@throws`
for each exposed entry point.
In `@Examples/blossom_example.cc`:
- Around line 78-84: Replace standard library containers in the Example with
Aleph equivalents: change struct Scenario's members from string slug/title and
vector<Node_Pos> nodes and vector<pair<size_t,size_t>> edges to use Aleph types
(keep std::string only if Aleph has none; otherwise use Aleph string type) and
use Aleph::DynArray<Node_Pos> for nodes and
Aleph::DynArray<Aleph::Pair<size_t,size_t>> or DynArray of a small Aleph tuple
for edges; likewise replace the extracted pairs container and the matched_set
usages with Aleph::DynArray and Aleph::DynSetTree respectively (update code
referencing Scenario::nodes, Scenario::edges, the extracted pairs variable, and
matched_set to the new Aleph types and adjust construction/iteration
accordingly).
In `@Examples/hungarian_example.cc`:
- Around line 144-146: The printed label "Assignments (2 tasks left
unassigned):" is hardcoded and fragile; update the output to compute the
unassigned count from ha.get_assignments() rather than using the literal "2".
Retrieve the vector/collection returned by ha.get_assignments(), compute how
many tasks are unassigned (e.g., by comparing number of tasks to number of
assignments or counting sentinel values in the assignments), and print that
computed count in the cout label before iterating over the assignments; refer to
ha and its get_assignments() result when locating the code to change.
- Around line 102-108: Remove the redundant mat.allocate() call: the DynMatrix
constructor already sets up internal storage via set_dimension() so calling
allocate() on the DynMatrix instance (mat.allocate()) is unnecessary here
because the code immediately writes every entry; delete the allocate()
invocation to avoid redundant pre-reservation while leaving the constructor and
the subsequent loop that fills mat(i, j) unchanged.
In `@Examples/planarity_test_example.cc`:
- Around line 315-330: After creating the ofstream objects (fj, fd, fg, fx) and
writing json/dot/graphml/gexf, check each stream's state (e.g., is_open() and
good()) before printing the "written to" messages for json_path, dot_path,
graphml_path, and gexf_path; if a stream failed to open or has errors, emit an
error message (or skip the success message) instead of unconditionally
announcing the file was written. Target the ofstream variables fj, fd, fg, fx
and the corresponding path variables json_path, dot_path, graphml_path,
gexf_path when adding these guards.
In `@K_Shortest_Paths.H`:
- Around line 354-372: The bounds check on arcs in same_prefix_nodes is
off-by-one and can allow spur_index == a.arcs.size(), enabling an OOB when
callers (e.g. code that does p_snapshot.arcs[spur_index]) access that element;
update the guard in same_prefix_nodes to reject cases where a.arcs.size() <=
spur_index or b.arcs.size() <= spur_index so that arcs[spur_index] is guaranteed
to exist before any access using spur_index.
- Around line 1150-1179: Add comprehensive Doxygen to the Yen_K_Shortest_Paths
template and its operator(): document the template parameters with `@tparam`
entries for GT, Distance, and SA; add `@brief` for the class and for operator();
add `@param` for g, source, target, and k; add `@return` describing the
DynList<K_Shortest_Path_Item<GT, typename Distance::Distance_Type>> result; add
`@throws` for possible exceptions propagated by yen_k_shortest_paths and any
precondition violations; and include `@complexity` and `@note` tags to match the
style and detail used in Eppstein_K_Shortest_Paths::operator(). Ensure the
operator() comment sits immediately above Yen_K_Shortest_Paths::operator() and
references yen_k_shortest_paths<GT, Distance, SA> where appropriate.
- Around line 1090-1097: The current early-return in eppstein_k_shortest_paths
(the source==target block that creates a trivial Path<GT>(g, source), appends an
Item with total_cost 0 and returns result) prevents generation of loopy cycles
for k>1 because generate_general_deviation_candidates exits when
base_path.arcs.is_empty(); either document this limitation in the function's
`@note` as "general (loopy) variant does not enumerate cycles when source==target"
or modify the initialization to seed cycle candidates by iterating the outgoing
arcs from source and pushing those one-step cycle starters into the main
candidate queue before entering the main loop (so
generate_general_deviation_candidates can explore s→a→…→s cycles); update
references to eppstein_k_shortest_paths, generate_general_deviation_candidates,
and the base_path.arcs.is_empty() behavior accordingly.
- Around line 875-897: Update the Doxygen block for yen_k_shortest_paths to
include an `@complexity` clause (matching the same complexity text used in
eppstein_k_shortest_paths) and add a `@note` about thread-safety stating that the
function may mutate the graph internally (it uses const_cast / Cookie_Saver) and
therefore is not thread-safe when the same graph is accessed concurrently;
ensure the wording parallels the eppstein_k_shortest_paths entries for
consistency.
- Around line 150-183: Remove the dead class In_Iterator_Adapter: delete the
entire template<class GT, class SA> class In_Iterator_Adapter definition and its
methods (constructor, has_curr, next_ne, get_current_arc_ne, get_tgt_node) since
it is unused; ensure no other code references In_Iterator_Adapter or its members
(it wraps In_Iterator<GT,SA> and delegates to methods like has_curr(),
next_ne(), get_curr_ne(), get_tgt_node()), and run a build to confirm no
remaining dependencies.
- Around line 473-490: The equality check cand == dist_node (and the analogous
cand == dist_u in the digraph code) is fragile for floating-point Cost_Type;
change the fallback arc-selection to use an epsilon-based comparison or track
the best-cost candidate instead: iterate the Node_Arc_Iterator loop (referencing
selected, index.dist_to_target, Node_Arc_Iterator<GT, SA>, get_current_arc_ne,
shortest_path_detail::checked_add, distance(arc), dist_node) and either (a)
treat cand as matching if fabs(cand - dist_node) <= eps where eps is derived
from Cost_Type (or relative tolerance), or (b) keep the arc with the minimal
|cand - dist_node| and select it after the loop; apply the same change to the
digraph counterpart that uses cand == dist_u so no correct tree arc is missed or
a wrong arc chosen.
---
Outside diff comments:
In `@Tests/geom_algorithms_test_boolean_3d_serializer_aabb_misc.cc`:
- Around line 1604-1654: The two tests
ConvexPolygonOffsetCollinearConsecutiveVertices and
ConvexPolygonOffsetLargeInwardOffset assert correct behavior but are known to
fail; either disable them by renaming the test cases to prefix DISABLED_ (e.g.,
DISABLED_ConvexPolygonOffsetCollinearConsecutiveVertices,
DISABLED_ConvexPolygonOffsetLargeInwardOffset) so CI skips them, or convert them
into regression-guard checks that assert the current buggy behavior (e.g.,
replace EXPECT_LT(offset_area, orig_area) with EXPECT_GT(offset_area, orig_area)
in ConvexPolygonOffsetCollinearConsecutiveVertices and replace
EXPECT_LT(result.size(), 3u) with EXPECT_GE(result.size(), 3u) in
ConvexPolygonOffsetLargeInwardOffset) and add comments referencing the bug id;
pick one approach consistently and keep test names (or comments) indicating that
these are temporary until ConvexPolygonOffset is fixed.
In `@Tests/k2tree_test.cc`:
- Line 462: The EXPECT_GE(result.size(), 0) assertion is trivially true because
result.size() is unsigned; replace or remove it: either compute an
expected_count via a brute-force scan of the inserted points and assert
EXPECT_EQ(result.size(), expected_count) (or EXPECT_GE/EXPECT_LE with known
bounds), or remove the assertion and add a comment that the surrounding loop is
a crash-safety smoke test; locate and update the EXPECT_GE(result.size(), 0)
line and the surrounding test logic that produces result to implement the
meaningful comparison.
---
Duplicate comments:
In `@Dominators.H`:
- Around line 276-280: The method do_compute currently takes a const GT& but
uses const_cast to assign gptr and then mutates graph internals (gptr, root_ptr,
is_computed); change do_compute signature to take GT& (non-const), remove the
const_cast in the body and assign gptr = &g, keep root_ptr and is_computed
logic, and propagate the non-const GT& signature to all callers (update call
sites and any upstream declarations) so no const is cast away anywhere.
- Around line 304-315: The function returns early when num_reachable <= 1 before
allocating pred, but later code (dominance_frontiers) accesses pred[0], causing
out-of-bounds for single-node graphs; fix by allocating pred =
Array<DynList<long>>(static_cast<size_t>(num_reachable), DynList<long>()) before
the early-return (or alternatively add a guard that skips dominance_frontiers
when num_reachable <= 1), and ensure idom_arr[0] and is_computed remain set as
they are for the single-node case so subsequent callers see a valid, consistent
state.
In `@Examples/dominators_example.cc`:
- Around line 65-83: Update the ASCII control-flow diagram in
Examples/dominators_example.cc so the edges from then to join and else to join
are explicitly drawn (e.g., add arrows or connector lines showing "then → join"
and "else → join") rather than relying on implicit layout; locate the diagram
block containing the labels entry, cond, then, else, join, loop_header,
loop_body, exit and modify the lines around then and else to clearly show their
connections into join.
In `@Examples/min_mean_cycle_example.cc`:
- Around line 112-123: Sanity check for minimum mean cycle has been added by
computing witness_mean from r.cycle_total_cost and r.cycle_length and comparing
it to r.minimum_mean; no change required — keep the block that computes
witness_mean, the std::abs comparison against static_cast<long
double>(r.minimum_mean), and the diagnostic cerr followed by abort() as-is (look
for witness_mean, r.cycle_total_cost, r.cycle_length, r.minimum_mean, cerr,
abort).
In `@Examples/planarity_test_example.cc`:
- Around line 62-78: Move the Doxygen block so it precedes the template
declaration rather than sitting between `template <class GT>` and the function
signature; specifically, relocate the comment that documents `print_result` to
immediately before the `template <class GT>` line for `print_result` (and do the
same for the Doxygen comment for `print_drawing`) so Doxygen associates the
documentation with the templated functions.
In `@Tests/ah-functional.cc`:
- Around line 1537-1545: The Doxygen comment and the FloatToleranceEqual
constructor are placed at column 0 rather than as indented members of the
struct; move the entire /** ... */ block and the constructor definition
FloatToleranceEqual(float tol = 0.001f) : tolerance(tol) {} so they are properly
indented inside the struct body (aligned with other member declarations) so
Doxygen associates the comment with the constructor/member `tolerance`.
In `@Tests/blossom_test.cc`:
- Around line 110-120: The helper template build_graph currently takes a
std::vector for edges and other tests use std::vector/STD buffers; replace those
with Aleph containers (e.g., DynArray<std::pair<size_t,size_t>> or Array where
fixed-size appropriate). Change the build_graph signature and callers to
accept/produce an Aleph DynArray instead of std::vector, populate aleph_edges
using push_back or proper DynArray resizing/assignment (avoid direct indexing on
un-sized container), and update the other helpers/DP buffers referenced around
the 158-197 ranges to use DynArray/Array equivalents so no std::vector remains.
Ensure types (pair<size_t,size_t>) match and preserve arc_info behavior when
forwarding to build_graph<GT>.
---
Nitpick comments:
In `@Examples/hungarian_example.cc`:
- Around line 137-141: The local variable ha (constructed as
Hungarian_Assignment<int> ha({...})) is not modified after construction—change
its declaration to const Hungarian_Assignment<int> ha({...}) to match the
pattern used in example_basic_assignment and to signal immutability; locate the
Hungarian_Assignment<int> ha construction and prefix it with const.
- Around line 104-107: Replace the C-style array 'data' with a std::array to
follow guidelines: change the declaration of data to std::array<std::array<int,
3>, 3> data = {{{{10,5,13}},{{3,9,18}},{{10,6,12}}}}; (or equivalent nested
std::array initializer), add `#include` <array> if missing, and keep the existing
loops that assign mat(i, j) = data[i][j]; so only the type of 'data' and its
initializer change while preserving its indexing and use in the mat population
code.
- Around line 57-80: The cost matrix is duplicated as a literal and a C-style
int[4][4]; define a single constexpr std::array<std::array<int,4>,4> (e.g.
constexpr std::array<std::array<int,4>,4> cost = {{ {...}, {...}, {...}, {...}
}}), use that same constexpr when constructing Hungarian_Assignment<int> (pass
cost) and use cost[r][c] for the detailed output; replace the C-style costs
array with this std::array and update references to ha, Hungarian_Assignment,
and costs accordingly so there is a single compile-time source of truth.
In `@Examples/min_mean_cycle_example.cc`:
- Line 144: Replace the explicit pointer declaration "Arc * arc =
it.get_curr();" with type-deducing auto to follow project C++20 style: use "auto
* arc = it.get_curr();" (i.e., change the declaration in
Examples/min_mean_cycle_example.cc where Arc, arc and it.get_curr() are used) so
the compiler deduces the pointer type from the iterator's get_curr() return
type.
- Around line 156-168: The anonymous namespace closing brace '}' is fused with
the Doxygen comment for main (the line starting with "} /**"), which is
confusing; edit the closing brace for the anonymous namespace (the lone '}') so
it sits on its own line followed by a blank line, then keep the Doxygen block
(/** ... `@brief` ... */) for main() starting on the next line; this affects the
anonymous namespace closing token and the main() Doxygen comment near the end of
Examples/min_mean_cycle_example.cc.
- Around line 120-122: Replace the bare abort() call with the ah_fatal_error()
macro so the fatal condition is reported through Aleph's error infrastructure;
specifically, in the block that prints the sanity check using witness_mean and
r.minimum_mean (the cerr << ... lines and abort()), call ah_fatal_error() with a
descriptive message including witness_mean and r.minimum_mean instead of abort()
so the error is handled by the project’s logging/diagnostics.
In `@Examples/planarity_test_example.cc`:
- Around line 180-448: main is long because it contains four large inline demo
blocks; extract each block into its own helper function (e.g.,
demo_planar_sample(UGraph&, const Planarity_Test_Options&, const fs::path&),
demo_dense_sample(...), demo_k33_certificate(const fs::path&, const
Planarity_Test_Options&), demo_digraph_sample(DGraph&, const
Planarity_Test_Options&)) and move the corresponding logic (graph construction,
planarity_test calls, printing, certificate file writing and validation) into
those functions, preserving use of symbols like Planarity_Test_Options,
strict_lr_opts, planarity_test, nonplanar_certificate_to_json/dot/graphml/gexf,
validate_nonplanar_certificate, planar_geometric_drawing, planar_dual_metadata;
make each helper take only the inputs it needs (output_dir and opts where
appropriate), return void (or a small result struct if needed), and replace the
inline blocks in main with simple calls to these helpers to improve readability.
In `@K_Shortest_Paths.H`:
- Around line 251-255: Path_Snapshot's template parameter Cost_Type is unused;
change the declaration of struct Path_Snapshot to remove the Cost_Type template
parameter (so it only depends on GT) and then update all call sites that
reference Path_Snapshot with two template args (e.g., make_path_snapshot<GT,
Cost_Type>, same_prefix_nodes<GT, Cost_Type>, and any other instantiations) to
pass only GT (e.g., make_path_snapshot<GT>, same_prefix_nodes<GT>), adjusting
forward declarations and using statements accordingly to ensure builds still
find Path_Snapshot<GT>.
- Around line 1007-1016: The current linear scan in yen_k_shortest_paths and
eppstein_k_shortest_paths that finds the best candidate by iterating over
candidates (State vector and indices best_idx/get_last/remove_last) makes
extractions O(k) and yields O(k²) overall; replace the candidates container with
an Aleph min-heap (use the provided heap API for push and pop/min operations) so
you push new State entries into the heap and extract the minimum via heap_pop
(O(log k)) instead of the scan+swap_last/remove_last pattern, adjust any code
that relied on get_last/remove_last to use heap_pop/heap_top, and update the
`@complexity` doc for eppstein_k_shortest_paths if you cannot complete the heap
refactor immediately.
- Around line 800-871: The deduplication via contains_path in
generate_general_deviation_candidates (and the Yen inner loop that also calls
contains_path) is O(k) per candidate; replace it with a path-signature set:
compute a compact signature for a Path_State by hashing its arc-pointer sequence
(e.g., XOR/combine reinterpret_cast<size_t>(arc) across candidate.arcs and
include arc count or another small discriminator), maintain a DynSetTree<size_t>
or OLhashTable<size_t> called seen_hashes alongside candidates/accepted, check
seen_hashes before appending, and insert the signature when you append a
candidate; update both generate_general_deviation_candidates (where
candidates.append(candidate) is called) and the Yen candidate insertion site to
use the signature-set check instead of contains_path to reduce dedup complexity
to amortized O(path_length).
In `@Tests/geom_algorithms_test_boolean_3d_serializer_aabb_misc.cc`:
- Around line 1261-1274: Replace the weak boolean assertions with richer failure
output: in the test SegmentEnlargeDiagonal, keep using Segment::length() to
compare lengths after enlarge_src/enlarge_tgt (on s_copy and s_copy2) but change
EXPECT_TRUE(s_copy.length() > s.length()) and EXPECT_TRUE(s_copy2.length() >
s.length()) to EXPECT_GT(s_copy.length(), s.length()) and
EXPECT_GT(s_copy2.length(), s.length()) respectively so failures print both
values and aid debugging; target the assertions involving s_copy, s_copy2,
Segment::length(), enlarge_src, and enlarge_tgt.
In `@Tests/k2tree_test.cc`:
- Around line 44-46: Replace all uses of std::vector<Point> with Aleph's
DynList<Point> (occurrences around the symbols in Tests/k2tree_test.cc at the
noted spots: lines ~174, ~582, ~613, ~736) and replace
std::set<std::pair<double,double>> with DynSetTree<std::pair<double,double>>
(occurrences around lines ~662, ~935, ~943); remove the `#include` <vector> and
`#include` <set> headers and add the corresponding Aleph container headers (e.g.,
the DynList and DynSetTree headers used in our codebase) so the file uses Aleph
containers everywhere while leaving <cmath> intact. Ensure template parameters
and any construction/iteration sites are updated to the DynList/DynSetTree APIs
if needed.
Refactor common test helpers for graph construction and functional algorithms into a dedicated header, `test_graph_helpers.h`. This reduces code duplication across test files for LCA, HLD, and Blossom algorithms. Additionally, update comments in `ahFunctional.H` and `Blossom_Weighted.H` for clarity, and refine input validation in `Min_Cost_Matching.H` and `Hungarian.H` to use `__int128` for intermediate cost calculations, preventing overflow with `LLONG_MIN`. Update Planarity example and Blossom example outputs to use `std::cout` and `std::string` formatting instead of `printf`.
refactor: Convert nightly Gephi comparison to weekly. This change involves renaming related files and update corresponding scripts to reflect a weekly execution schedule for Gephi comparison and reporting. test: Refactor cut nodes and blossom tests test: Improve graph construction and assertions in cut nodes and blossom tests. The test utilities for building graphs and verifying matching or cut node properties have been refactored for clarity and robustness, including changes to node insertion logic and assertion helpers. docs: Update README for weekly Gephi comparison docs: Update README to reflect weekly Gephi comparison script. The README has been updated to reference the new 'weekly' comparison script instead of the 'nightly' one.
Modernizes several C++ example files to use C++11/17 features like std::array and std::format, improving readability and consistency. Also introduces `test_graph_helpers.h` to centralize graph building utilities for tests.
docs: Introduce GitHub Pages documentation workflow refactor(K_Shortest_Paths): Reformat Doxygen comments chore: Update .gitignore with new patterns and generated files
Introduces Ruby scripts for Gephi nightly artifact comparison, including render profiles and templates. Also adds a K33 non-planar graphml fixture for testing. Refactors dominators_example.cc for clarity by moving print_frontier to a local namespace.
Refactor run_external_certificate_validator to use a new run_script_cmd function, which sets GITHUB_OUTPUT to empty to prevent scripts from writing to the CI output file. This improves test reliability for external script invocations.
Comment on lines
+224
to
+247
| DynSetTree<std::pair<size_t, size_t>> matched_set(output.matched_pairs); | ||
|
|
||
| std::ofstream out(file_path); | ||
| if (not out) | ||
| { | ||
| std::cerr << "Cannot write file: " << file_path << "\n"; | ||
| return; | ||
| } | ||
|
|
||
| out << "\\documentclass[tikz,border=10pt]{standalone}\n"; | ||
| out << "\\usepackage{tikz}\n"; | ||
| out << "\\usetikzlibrary{arrows.meta,calc}\n"; | ||
| out << "\\begin{document}\n"; | ||
| out << "\\begin{tikzpicture}[x=1cm,y=1cm]\n"; | ||
|
|
||
| out << " \\path[shade, left color=cyan!6, right color=blue!5] " | ||
| << "(-4.6,-3.4) rectangle (4.6,3.4);\n"; | ||
|
|
||
| out << " \\node[font=\\bfseries\\large] at (0,3.05) {" | ||
| << s.title << "};\n"; | ||
|
|
||
| out << " \\node[font=\\small] at (0,2.65) {objective: " | ||
| << (max_cardinality ? "max-cardinality then max-weight" : "pure max-weight") | ||
| << "};\n"; |
There was a problem hiding this comment.
DynSetTree doesn’t appear to use a contains() API elsewhere in this PR (tests use exist() / search()); this will likely fail to compile. Consider switching to the set’s supported membership check (e.g., matched_set.exist(std::make_pair(a, b)) or matched_set.search(...) != nullptr) and, if DynSetTree doesn’t support constructing from output.matched_pairs, populate matched_set via inserts in a loop.
Comment on lines
+265
to
+278
| for (const auto & [u_raw, v_raw, w] : s.edges) | ||
| { | ||
| size_t u = u_raw; | ||
| size_t v = v_raw; | ||
| size_t a = std::min(u, v); | ||
| size_t b = std::max(u, v); | ||
| const bool matched = matched_set.contains(std::make_pair(a, b)); | ||
|
|
||
| out << " \\draw[" << (matched ? "match" : "edge") << "] (n" | ||
| << u << ") -- (n" << v << ");\n"; | ||
|
|
||
| out << " \\node[wlabel] at ($(n" << u << ")!0.5!(n" << v << ")$) {" | ||
| << w << "};\n"; | ||
| } |
There was a problem hiding this comment.
DynSetTree doesn’t appear to use a contains() API elsewhere in this PR (tests use exist() / search()); this will likely fail to compile. Consider switching to the set’s supported membership check (e.g., matched_set.exist(std::make_pair(a, b)) or matched_set.search(...) != nullptr) and, if DynSetTree doesn’t support constructing from output.matched_pairs, populate matched_set via inserts in a loop.
| cout << " (no path found)\n"; | ||
| cout << '\n'; | ||
| } | ||
| } /** |
There was a problem hiding this comment.
The closing brace for the anonymous namespace and the following Doxygen comment are on the same line (} /**), which harms readability and can confuse documentation tooling. Put the } on its own line, add a blank line, and then start the comment block.
Suggested change
| } /** | |
| } | |
| /** |
Comment on lines
+168
to
+174
| * - full result with witness cycle, | ||
| * - value-only API reporting only the minimum mean, | ||
| * - filtered run that blocks a specified arc (D→E) via the Arc_Visible predicate. | ||
| * Results are printed to standard output. | ||
| * | ||
| * @return int Exit status (0 on success). | ||
| */ |
There was a problem hiding this comment.
The list items in the Doxygen comment are misaligned (extra leading space before * on lines 168–171). Align the leading * consistently so Doxygen renders the bullet list correctly.
Suggested change
| * - full result with witness cycle, | |
| * - value-only API reporting only the minimum mean, | |
| * - filtered run that blocks a specified arc (D→E) via the Arc_Visible predicate. | |
| * Results are printed to standard output. | |
| * | |
| * @return int Exit status (0 on success). | |
| */ | |
| * - full result with witness cycle, | |
| * - value-only API reporting only the minimum mean, | |
| * - filtered run that blocks a specified arc (D→E) via the Arc_Visible predicate. | |
| * Results are printed to standard output. | |
| * | |
| * @return int Exit status (0 on success). | |
| */ |
Update planarity_test.cc to include more detailed diagnostics and improve readability. Adjust planarity_test_example.cc to enhance the summary output and clarify comments.
Refactor code in Dominators.H and HLD.H for clarity and better handling of node mappings and subtree size calculations.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Examples
Tests
Documentation