Conversation
Refactor algorithm documentation by adding complexity analysis and examples to various header files. This includes Bellman-Ford, LIS, Matrix Chain, Subset Sum, Knapsack, and Min Cost Matching, among others.
This large commit introduces a complete compiler frontend, including: - Diagnostics, Source, AST, Lexer, Parser, Sema, Token, Typed Sema, and Types modules. - Corresponding example and test files for each new component. - Updates to build configurations and documentation to reflect the new functionality. - Minor adjustments to utility and script files.
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 a full compiler front-end to Aleph (source manager, diagnostics, tokens/lexer, AST, parser, scopes/sema, type graph, constraint unifier, typed semantic pass) with examples, tests, docs, and CMake/test registration; also includes large refactors (suffix structures, parse_utils), Gephi template/CI updates, numerous documentation/metadata edits, and small build/install/listing changes (.clang-format, CMakeLists). Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SM as Source_Manager
participant DX as Diagnostic_Engine
participant Lexer as Compiler_Lexer
participant Parser as Compiler_Parser
participant Sema as Compiler_Semantic_Analyzer
participant Typed as Compiler_Typed_Semantic_Analyzer
participant Unifier as Compiler_Type_Unifier
User->>SM: add_virtual_file(name, text)
User->>DX: Diagnostic_Engine(SM)
User->>Lexer: new Lexer(SM, file_id, DX)
loop Tokenize
Lexer->>Lexer: peek()/next()
alt invalid lexeme
Lexer->>DX: emit error (LEX###)
end
end
User->>Parser: new Parser(ctx, SM, file_id, DX)
Parser->>Lexer: peek()/next()
alt parse error
Parser->>DX: emit (PAR###)
Parser->>Parser: synchronize()
end
Parser->>User: AST (Module)
User->>Sema: analyze_module(AST)
Sema->>Sema: build symbols / resolve names
alt semantic error
Sema->>DX: emit (SEM###)
end
User->>Typed: analyze_module(AST)
Typed->>Typed: generate type constraints
Typed->>Unifier: solve(constraint_set)
Unifier->>Unifier: unify (occurs-check, rigid checks)
Unifier->>Typed: substitution
Typed->>User: inferred types / dump
User->>DX: render_plain()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a new “compiler front-end support” layer (source spans, diagnostics, tokens/lexer/parser, semantic analysis, types/constraints) and updates the planarity external validator’s optional Gephi checks to be more portable by probing executable availability in PATH rather than running gephi --version.
Changes:
- Introduces new compiler-oriented utility headers (
ah-source.H,ah-diagnostics.H,tpl_scope.H,tpl_constraints.H) and compiler modules (token/lexer/parser/sema/types/typed-sema) plus examples, docs, and tests. - Adjusts Gephi validation templates/scripts/workflow/docs to use PATH/executable probes and updates CI/tests accordingly.
- Expands/refreshes documentation across multiple algorithm headers and tweaks formatting/tooling config.
Reviewed changes
Copilot reviewed 62 out of 62 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tpl_scope.H | Adds a generic nested scope stack used by compiler tooling. |
| tpl_constraints.H | Adds equality constraints, substitutions, and a compiler type unifier. |
| tikzgeom_scene.H | Adds Doxygen file header documentation. |
| scripts/planarity_gephi_templates.json | Switches Gephi smoke checks to PATH probes via Ruby one-liners. |
| scripts/planarity_certificate_validator.rb | Updates Gephi default behavior and template catalog entries for portability. |
| scripts/planarity_certificate_ci_batch.rb | Aligns default Gephi command behavior with validator changes. |
| primality.H | Adds author tag to Doxygen header comment. |
| pollard_rho.H | Adds author tag to Doxygen header comment. |
| modular_linalg.H | Adds author tag to Doxygen header comment. |
| modular_arithmetic.H | Adds author tag to Doxygen header comment. |
| hash-fct.H | Adds Doxygen header comment; minor signature constness tweak. |
| docs/compiler_frontend_support.md | New Spanish guide describing compiler front-end support layer. |
| docs/compiler_frontend_support.en.md | New English guide describing compiler front-end support layer. |
| blossom_weighted_mwmatching.H | Adds author tag to Doxygen header comment. |
| ah-source.H | New source manager/span/snippet utilities for compiler-style tooling. |
| ah-diagnostics.H | New buffered diagnostic engine with deterministic plain-text rendering. |
| Tests/planarity_test.cc | Makes Gephi availability check portable without executing Gephi. |
| Tests/compiler_types_test.cc | Adds unit tests for the compiler type context. |
| Tests/compiler_typed_sema_test.cc | Adds unit tests for typed semantic analysis/inference. |
| Tests/compiler_token_test.cc | Adds unit tests for token metadata/helpers. |
| Tests/compiler_source_test.cc | Adds unit tests for source manager/span/snippet behavior. |
| Tests/compiler_sema_test.cc | Adds unit tests for name resolution and semantic diagnostics. |
| Tests/compiler_scope_test.cc | Adds unit tests for nested scope stack behavior. |
| Tests/compiler_parser_test.cc | Adds unit tests for parser behavior and diagnostics. |
| Tests/compiler_lexer_test.cc | Adds unit tests for lexer behavior, spans, and diagnostics. |
| Tests/compiler_diagnostics_test.cc | Adds unit tests for diagnostic builder/rendering determinism. |
| Tests/compiler_constraints_test.cc | Adds unit tests for constraints/unification behavior. |
| Tests/compiler_ast_test.cc | Adds unit tests for AST arena/dump helpers. |
| Tests/CMakeLists.txt | Registers new compiler tooling tests in CMake. |
| Subset_Sum.H | Enhances documentation with complexity table and example. |
| README.md | Documents compiler tooling and updates Gephi validation usage text. |
| README.es.md | Spanish README updates matching README.md changes. |
| Planarity_Test.H | Adds author tag to Doxygen header comment. |
| Min_Mean_Cycle.H | Adds author tag to Doxygen header comment. |
| Min_Cost_Matching.H | Adds example and author tag to Doxygen header comment. |
| Matrix_Chain.H | Enhances documentation with complexity table and example. |
| LIS.H | Enhances documentation with variants/complexity/example sections. |
| Knapsack.H | Enhances documentation with complexity table and example. |
| K_Shortest_Paths.H | Updates docs (complexity + example) and adds author tag. |
| Hungarian.H | Updates docs (complexity + example). |
| Examples/planarity_test_example.cc | Updates template id reference to ruby-based probe. |
| Examples/compiler_types_example.cc | Adds example for types + constraints/unifier. |
| Examples/compiler_typed_sema_example.cc | Adds example for typed semantic analysis. |
| Examples/compiler_sema_example.cc | Adds example for semantic analysis. |
| Examples/compiler_parser_example.cc | Adds example for parsing and AST dumping. |
| Examples/compiler_lexer_example.cc | Adds example for lexing and token dump. |
| Examples/compiler_diagnostics_example.cc | Adds example for diagnostics rendering. |
| Compiler_Types.H | Adds compiler type graph/context with stable ids and pretty-printing. |
| Compiler_Token.H | Adds token model, keyword classification, and operator metadata. |
| Compiler_Sema.H | Adds name-resolution and basic semantic checks over the AST. |
| Compiler_Parser.H | Adds recursive-descent parser using the new lexer/AST. |
| Compiler_Lexer.H | Adds lexer producing tokens with stable spans and diagnostics. |
| CMakeLists.txt | Adds new compiler-tooling headers to header install/build lists. |
| Blossom_Weighted.H | Adds author tag to Doxygen header comment. |
| Blossom.H | Updates docs (complexity + example). |
| Bellman_Ford.H | Restructures documentation and updates example/complexity table. |
| .github/workflows/planarity_gephi_nightly.yml | Changes nightly Gephi validation step to executable availability probe. |
| .clang-format | Adjusts formatting rules for case labels. |
Comments suppressed due to low confidence (6)
ah-diagnostics.H:1
Source_Span::is_valid()only checksfile_id != 0andbegin <= end, but it does not guarantee thatfile_idis registered inSource_Manager. As a result,render_plain()can throw (viabegin_position()/snippet()) when a diagnostic carries a span with an unregisteredfile_id. Consider hardening rendering by (mandatory) validatinglabel.span.file_id <= sources->num_files()(and optionally span offsets) before rendering, or by catchingstd::out_of_rangeduring rendering and emitting a fallback location like<unknown>.
scripts/planarity_certificate_validator.rb:1gephi_path_probe_template()interpolatesexecutable_namedirectly into a command string, which is unsafe if that value can ever come from external input (e.g., JSON template catalogs or CLI-provided templates) and the command is executed through a shell. Prefer (mandatory) shell-escaping withShellwords.escape(executable_name)when building the string, or better, avoid shell parsing entirely by executing Ruby with an argv array (Open3 with array form) and passingexecutable_nameas a separate argument.
Tests/planarity_test.cc:1std::filesystem::exists()/is_regular_file()can throw on certain permission/IO errors. Since this is used in a test helper, an unexpected exception can make CI flaky on hardened environments. Use thestd::error_codeoverloads forexists/is_regular_file(orstatus) and treat errors as 'not found'.
tpl_constraints.H:1Compiler_Type_Substitution::apply()allocates and appends new tuple/function nodes intoCompiler_Type_Contextwhenever a child changes, but it does not memoize results. Repeatedapply()calls (e.g., duringdump_substitution()or repeated inference dumps) can grow the type context with duplicate structurally-identical nodes. Consider (recommended) adding memoization keyed by inputidunder the current substitution, or interning inCompiler_Type_Context(structural hashing) to reuse existing tuple/function nodes when identical.
tpl_scope.H:1- This header uses
size_t(e.g., in loops and return types) but does not include<cstddef>. Relying on transitive includes from other headers is not guaranteed and can break compilation in stricter environments. Consider (mandatory) adding# include <cstddef>to make the header self-contained.
tpl_constraints.H:1 - This header uses
size_textensively but does not explicitly include<cstddef>. While some standard headers often pull it in, that’s not something to rely on for a public header. Consider (mandatory) adding# include <cstddef>to ensure portability and self-contained compilation.
![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: 16
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)
scripts/planarity_certificate_validator.rb (1)
530-545:⚠️ Potential issue | 🟠 MajorMust fix:
resolve_executable()fails to find Windows executables with different extensions.The function will not find
ruby.exewhen searching for barerubyon Windows, breaking the portable ruby render templates added in this PR. Line 534's separator check only recognizes/and won't detect backslash paths, but the primary issue is that bare executable names likerubyare resolved without checking for common Windows extensions (.exe,.bat,.cmd,.com).The suggested fix using
PATHEXTis incorrect—Ruby's native executable lookup uses hardcoded extensions (viadln_find_exe_r), not thePATHEXTenvironment variable. Instead, try these common Windows extensions for bare command names:Suggested fix
def resolve_executable(cmd) raw = cmd.to_s return nil if raw.empty? - if raw.include?(File::SEPARATOR) - return raw if File.file?(raw) and File.executable?(raw) - return nil - end - - ENV.fetch("PATH", "").split(File::PATH_SEPARATOR).each do |dir| - candidate = File.join(dir, raw) - return candidate if File.file?(candidate) and File.executable?(candidate) - end + separators = [File::SEPARATOR, File::ALT_SEPARATOR].compact + if separators.any? { |sep| raw.include?(sep) } + return raw if File.file?(raw) and File.executable?(raw) + return nil + end + + exts = [""] + if File::ALT_SEPARATOR && File.extname(raw).empty? + exts.concat([".exe", ".bat", ".cmd", ".com"]) + end + + ENV.fetch("PATH", "").split(File::PATH_SEPARATOR).each do |dir| + exts.each do |ext| + candidate = File.join(dir, "#{raw}#{ext}") + return candidate if File.file?(candidate) and File.executable?(candidate) + end + end nil end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/planarity_certificate_validator.rb` around lines 530 - 545, The resolve_executable method fails to find Windows executables because it only treats paths containing File::SEPARATOR as "absolute/relative" and, for bare names, never tries Windows executable extensions; update resolve_executable to treat either File::SEPARATOR or File::ALT_SEPARATOR (backslash) as indicating a path and, when searching PATH for a bare command (e.g., "ruby"), try the common Windows extensions [".exe", ".bat", ".cmd", ".com"] by appending each extension to the candidate when the plain candidate isn't an executable, returning the first matching File.file? && File.executable?; keep the existing behavior on non-Windows platforms.
🟡 Minor comments (8)
Bellman_Ford.H-49-57 (1)
49-57:⚠️ Potential issue | 🟡 MinorDefine
startin the Doxygen example before first use.The snippet uses
start(Line 56) without declaring it, which makes the example harder to follow/copy.📘 Suggested doc tweak
* Bellman_Ford bf(g); + * auto start = /* valid source node in g */; * * // Check for negative cycles and compute shortest paths from 'start' * if (bf.has_negative_cycle(start)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Bellman_Ford.H` around lines 49 - 57, The Doxygen example uses start without declaring it; add a declaration (e.g., a vertex identifier compatible with List_Digraph<Node, Arc>) before calling Bellman_Ford and has_negative_cycle so the snippet is copy-pasteable — for example declare a variable named start of the appropriate node/vertex type (matching List_Digraph<Node, Arc>) and initialize it to a valid vertex in the constructed graph before calling bf.has_negative_cycle(start) and bf.build_negative_cycle().Matrix_Chain.H-58-67 (1)
58-67:⚠️ Potential issue | 🟡 MinorShould fix: qualify API names in example for copy-paste safety
The example uses unqualified calls to
matrix_chain_order/matrix_chain_min_cost, but APIs are inAleph. Qualifying names avoids ambiguity in docs/examples.Suggested improvement
- * auto res = matrix_chain_order(dims); + * auto res = Aleph::matrix_chain_order(dims); @@ - * size_t cost = matrix_chain_min_cost(dims); // Returns 5000 + * size_t cost = Aleph::matrix_chain_min_cost(dims); // Returns 5000🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Matrix_Chain.H` around lines 58 - 67, The example in the header uses unqualified function names matrix_chain_order and matrix_chain_min_cost which can conflict; update the example to call Aleph::matrix_chain_order and Aleph::matrix_chain_min_cost (or add a using declaration explicitly) so the snippet is copy-paste safe and unambiguous; change occurrences of matrix_chain_order(...) and matrix_chain_min_cost(...) in the example to their qualified forms Aleph::matrix_chain_order(...) and Aleph::matrix_chain_min_cost(...).K_Shortest_Paths.H-43-46 (1)
43-46:⚠️ Potential issue | 🟡 MinorAlign the Eppstein-style complexity claim with the current implementation.
The table states
O(E log V + k log k), but current extraction of the next candidate is a linear scan (Line 1121–Line 1123), so this bound is too optimistic as documented.As per coding guidelines "Document time complexity using Big-O notation in Doxygen comments".📝 Suggested doc adjustment
-| Eppstein-style | O(E log V + k log k) | O(E log V + k) | +| Eppstein-style | O(E log V + candidate-selection cost) (current impl uses linear candidate scans) | O(E + candidate pool) |Knapsack.H-45-64 (1)
45-64:⚠️ Potential issue | 🟡 MinorCorrect the complexity row and example result in the new docs.
Two inaccuracies in this block:
- Line 49 says 0/1 space is
O(capacity), butknapsack_01uses a 2D DP table (O(n * capacity)).- Line 62’s example states
optimal_value = 140(and cites{2,40}+{4,70}), which is incorrect forcapacity = 8.As per coding guidelines "Verify complexity analysis is included in Doxygen documentation".📘 Proposed documentation fix
-| 0/1 Knapsack | O(n * capacity) | O(capacity) | +| 0/1 Knapsack (with reconstruction) | O(n * capacity) | O(n * capacity) | +| 0/1 Knapsack (value only) | O(n * capacity) | O(capacity) | | Unbounded | O(n * capacity) | O(capacity) | | Bounded | O(capacity * sum(log c_i)) | O(capacity) | @@ - // res.optimal_value = 140 (items {2, 40} and {4, 70} or others) + // res.optimal_value = 130 (e.g. items {3, 50} and {5, 80})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Knapsack.H` around lines 45 - 64, Update the documentation for knapsack_01: correct the space complexity row to O(n * capacity) because knapsack_01 uses a 2D DP table, and fix the example result — for items Array<Knapsack_Item<size_t, int>> = {{2,40},{3,50},{4,70},{5,80}} with capacity = 8 the correct optimal_value is 130 (items with weights 5 and 3), so update the commented optimal_value and selected_items description accordingly.Hungarian.H-45-62 (1)
45-62:⚠️ Potential issue | 🟡 MinorShould fix: the new example does not match the exported API.
Line 59 calls
hungarian_min_assignment(...), but this header exposeshungarian_assignment(...). The snippet also builds aDynArray<DynArray<double>>, while the free-function entry point here takes aDynMatrix. As written, the example is misleading and likely non-compiling.📝 Suggested doc fix
- * DynArray<DynArray<double>> costs = { - * {10, 20, 30}, - * {40, 50, 10}, - * {20, 10, 60} - * }; + * DynMatrix<double> costs(3, 3); + * costs.allocate(); + * costs(0, 0) = 10; costs(0, 1) = 20; costs(0, 2) = 30; + * costs(1, 0) = 40; costs(1, 1) = 50; costs(1, 2) = 10; + * costs(2, 0) = 20; costs(2, 1) = 10; costs(2, 2) = 60; ... - * auto res = hungarian_min_assignment(costs); + * auto res = hungarian_assignment(costs);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Hungarian.H` around lines 45 - 62, The example uses the wrong API and types: it calls hungarian_min_assignment and constructs a DynArray<DynArray<double>>, but the header actually exposes hungarian_assignment and expects a DynMatrix; update the example to call hungarian_assignment and build/convert the input as a DynMatrix (or show how to construct a DynMatrix from the nested arrays) and adjust the expected result variable names (e.g., res.total_cost and res.row_to_col) to match the actual return type of hungarian_assignment; ensure the example compiles against the header by referencing hungarian_assignment and DynMatrix explicitly.Tests/planarity_test.cc-681-683 (1)
681-683:⚠️ Potential issue | 🟡 MinorUse non-throwing
std::filesystemoverloads to ensure deterministic PATH probing.Lines 682–683 call
fs::exists()andfs::is_regular_file()without error codes, which can throwstd::filesystem::filesystem_erroron permission denials or inaccessible PATH entries, making this test non-deterministic across environments. The code already demonstrates the correct pattern at line 689 withfs::status(candidate, ec).🔧 Suggested fix
const fs::path candidate = fs::path(dir) / name; - if (not fs::exists(candidate) or not fs::is_regular_file(candidate)) + std::error_code ec; + if (not fs::exists(candidate, ec) or ec) + continue; + if (not fs::is_regular_file(candidate, ec) or ec) continue;Also applies to: 689–691
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/planarity_test.cc` around lines 681 - 683, Replace the throwing calls to fs::exists(candidate) and fs::is_regular_file(candidate) with the non-throwing overloads that take a std::error_code (or use fs::status(candidate, ec) + fs::is_regular_file(status)) so that permission errors don’t throw; specifically, update the checks around the candidate path (variable candidate) to query existence and regular-file-ness using an std::error_code ec (or a fs::file_status status obtained via fs::status(candidate, ec)) and then test fs::exists(candidate, ec) or fs::is_regular_file(status) and skip on errors instead of letting fs::filesystem_error propagate.README.md-3961-3973 (1)
3961-3973:⚠️ Potential issue | 🟡 MinorUse the concrete exported constraint-set type name in the API table.
Constraint_Set<T>appears inconsistent with the compiler-typed API naming used elsewhere (Compiler_Type_Constraint_Set). This can mislead users copying the documented symbols.📝 Proposed doc fix
-| `tpl_constraints.H` | `Constraint_Set<T>`, `Compiler_Type_Unifier`, `Compiler_Type_Substitution` | Equality constraints, substitutions, structural unification, rigid variables, and occurs-check support | +| `tpl_constraints.H` | `Compiler_Type_Constraint_Set`, `Compiler_Type_Unifier`, `Compiler_Type_Substitution` | Equality constraints, substitutions, structural unification, rigid variables, and occurs-check support |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 3961 - 3973, The API table uses the templated/internal name Constraint_Set<T>, which is inconsistent with the exported/consumable symbol naming used elsewhere; change the entry to the public concrete name Compiler_Type_Constraint_Set (and if applicable mention Compiler_Type_Unifier and Compiler_Type_Substitution remain) so the README lists the actual exported type users should refer to instead of the internal template form; update the table row under tpl_constraints.H to read Compiler_Type_Constraint_Set and keep the rest of the description as-is.Tests/compiler_constraints_test.cc-102-122 (1)
102-122:⚠️ Potential issue | 🟡 MinorCover
solve()failure short-circuit semantics explicitly.This test only exercises the all-success path. Add one case where the first failing constraint stops processing so insertion-order behavior is locked down.
🧪 Proposed contract test
+TEST(CompilerConstraints, SolveStopsAtFirstFailureInInsertionOrder) +{ + Compiler_Type_Context ctx; + const auto alpha = ctx.make_type_variable("A"); + const auto rigid = ctx.make_type_variable("R", true); + + Compiler_Type_Constraint_Set constraints; + constraints.add(rigid, ctx.integer_type(), {}, "first fails"); + constraints.add(alpha, ctx.bool_type(), {}, "must not run"); + + Compiler_Type_Unifier unifier(ctx); + const auto result = unifier.solve(constraints); + + ASSERT_FALSE(result.ok()); + EXPECT_EQ(result.kind, Compiler_Unify_Result_Kind::Rigid_Variable); + EXPECT_EQ(unifier.apply(alpha), alpha); +}As per coding guidelines, "Unit tests must verify the method's contract (preconditions, postconditions, return values)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/compiler_constraints_test.cc` around lines 102 - 122, Add a failing-case to explicitly test solve() short-circuiting: build a new Compiler_Type_Context, create type variables via make_type_variable (e.g., A and B), then create a Compiler_Type_Constraint_Set where the first constraint added is impossible (e.g., add(A, ctx.string_type(), {}, "failing"), or a mismatched tuple) so solve(constraints) returns failure immediately; after calling Compiler_Type_Unifier::solve verify result is not ok and assert that constraints added after the failing one did not affect state by checking unifier.apply on subsequent variables still yields their original unconstrained types (use integer_type/string_type and symbols like add, solve, apply to locate the code to change).
🧹 Nitpick comments (5)
Blossom.H (1)
43-47: Resolve the complexity mismatch within this header.The new table (
O(V^2 * E),O(V)) conflicts with the API note at Line 433 (O(V^3),O(V + E)). Please make both sections consistent (and aligned with this implementation) to avoid misleading users.Suggested doc-only alignment
- * | Edmonds-Blossom | O(V^2 * E) | O(V) | + * | Edmonds-Blossom | O(V^3) | O(V + E) |And keep Line 433 aligned with the same complexity claim.
As per coding guidelines, “Verify complexity analysis is included in Doxygen documentation” and keep public API docs accurate and consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Blossom.H` around lines 43 - 47, Summary: The complexity table for Edmonds-Blossom in the Doxygen header conflicts with the API note; make them consistent with the implementation. Fix: update the table entry in Blossom.H (the Edmonds-Blossom complexity block) to match the implementation and the API Doxygen note by changing the time complexity to O(V^3) and the space complexity to O(V + E) (or if the implementation truly uses O(V^2 * E), change the API note and any Doxygen mention of O(V^3) to O(V^2 * E) and O(V) respectively); ensure both the table and the API Doxygen block (the public API note referencing Edmonds-Blossom) state the same complexities and reflect the actual algorithm complexity in the codebase.Examples/compiler_sema_example.cc (1)
58-62: Add a null-check before running semantic analysis.Line 61 assumes parsing always returns a valid module. A guard avoids accidental null dereference when the demo source is edited into an invalid form.
💡 Suggested hardening
Compiler_Parser parser(ctx, sm, file_id, &dx); const auto * module = parser.parse_module(); + if (module == nullptr) + { + dx.render_plain(std::cout); + return 1; + } Compiler_Semantic_Analyzer sema(&dx); sema.analyze_module(module);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/compiler_sema_example.cc` around lines 58 - 62, The code assumes parser.parse_module() always returns a valid pointer; add a null-check on the variable module before calling Compiler_Semantic_Analyzer::analyze_module to avoid dereferencing a null module. Locate the call to parser.parse_module() and the subsequent use in sema.analyze_module(module), and if module is null, log or report the parse failure and skip or early-return from the demo instead of calling sema.analyze_module.Examples/compiler_typed_sema_example.cc (1)
58-62: Guard against null module before semantic analysis.If parsing fails, Line 61 can dereference a null module pointer. A small guard makes the example resilient when users tweak input text.
💡 Suggested hardening
Compiler_Parser parser(ctx, sm, file_id, &dx); const auto * module = parser.parse_module(); + if (module == nullptr) + { + dx.render_plain(std::cout); + return 1; + } Compiler_Typed_Semantic_Analyzer typed(&dx); typed.analyze_module(module);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/compiler_typed_sema_example.cc` around lines 58 - 62, The example currently calls typed.analyze_module(module) without checking whether parser.parse_module() returned nullptr; add a null check for module after calling parse_module() and before constructing/using Compiler_Typed_Semantic_Analyzer so we don't dereference a null pointer — e.g., if module is null, log or print an error and return/exit early instead of calling Compiler_Typed_Semantic_Analyzer::analyze_module; refer to parse_module, module, Compiler_Typed_Semantic_Analyzer, and analyze_module to locate the relevant code.Tests/compiler_parser_test.cc (1)
44-178: Add an explicit empty-input parser test.This suite is strong, but it still misses the empty-source edge case (
""or whitespace-only), which is a high-value parser boundary.As per coding guidelines "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/compiler_parser_test.cc` around lines 44 - 178, Add a new unit test in Tests/compiler_parser_test.cc (e.g., TEST(CompilerParser, ParsesEmptyInput)) that adds an empty virtual file ("" or whitespace) via Source_Manager::add_virtual_file, constructs Diagnostic_Engine, Compiler_Ast_Context and Compiler_Parser (using same ctor pattern), calls parser.parse_module(), and asserts the returned module is non-null with zero functions and zero top-level statements and that dx.has_errors() is false; place the test alongside the other CompilerParser tests to cover the empty-source edge case.Tests/compiler_source_test.cc (1)
140-156: Make temp-file cleanup assertion-safe.Line 155 only runs on normal completion. If an
ASSERT_*fails earlier, the temp file can be leaked in CI workers.♻️ Proposed cleanup guard
TEST(SourceManager, LoadFileReadsContent) { namespace fs = std::filesystem; const fs::path path = make_temp_file_path("source_manager"); + struct Temp_File_Guard + { + fs::path p; + ~Temp_File_Guard() + { + std::error_code ec; + fs::remove(p, ec); + } + } guard{path}; + { std::ofstream out(path); out << "line1\nline2\n"; } Source_Manager sm; const auto id = sm.load_file(path.string()); EXPECT_EQ(sm.file_name(id), path.string()); EXPECT_EQ(sm.file_text(id), "line1\nline2\n"); - - fs::remove(path); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/compiler_source_test.cc` around lines 140 - 156, The test leaks the temp file if an assertion aborts before fs::remove(path); wrap the temp-file lifetime in a cleanup guard (e.g., a small RAII helper or scope guard) so the file created by make_temp_file_path("source_manager") is removed in the guard's destructor; update TEST SourceManager::LoadFileReadsContent to create the path then construct the guard (tied to path) before writing/using std::ofstream out and calling Source_Manager::load_file, ensuring cleanup always runs even on ASSERT/EXPECT failures.
🤖 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 376-377: The interpolated gephi_exe is inserted directly into a
shell command string (the "cmd" f-string) which risks command injection; change
the interpolation to use a safely escaped value (e.g. replace f'...
"{gephi_exe}"' with f'... {shlex.quote(gephi_exe)}') and add an import for shlex
at the top of the file, or alternatively build the command as a list to avoid
the shell; update the "cmd" entry that currently references gephi_exe so it uses
the escaped/quoted variable (shlex.quote(gephi_exe)) instead of the raw
gephi_exe.
In `@Compiler_AST.H`:
- Around line 824-850: The module dumper unconditionally dereferences fn (from
module->functions.access(i)), causing crashes for nullptr entries; before using
fn->name or fn->parameters in the loop, check if fn == nullptr and handle it
gracefully (e.g., print a placeholder like "Function(null)" with proper indent
and skip parameter/body dumping) or delegate to the existing
compiler_dump_function which already tolerates nullptr; ensure any calls to
Compiler_Ast_Detail::dump_stmt or accesses to fn->parameters are only done when
fn is non-null.
In `@Compiler_Types.H`:
- Around line 372-419: The tuple and function constructors (make_tuple_type,
both overloads, and make_function_type, both overloads) must validate their
child Compiler_Type_Id values before building and calling append_type: iterate
the members/parameters (and the function result_type) and reject any id == 0 or
not present in the type table by using/adding a helper like is_valid_type_id(id)
or a lookup function that checks the internal type registry; if any id is
invalid, do not call append_type and return a neutral/invalid Compiler_Type_Id
(or propagate an error) so invalid/stale ids are rejected at construction time
rather than stored in Compiler_Type. Ensure both the DynArray and
initializer_list overloads perform the same checks (the initializer_list path
calls list_to_array so validate after conversion) and add tests exercising
invalid ids.
In `@Matrix_Chain.H`:
- Around line 63-67: The example doc has wrong numeric results for dims =
{10,30,5,60,10}: update the comment values so res.parenthesization = "((A1 A2)
(A3 A4))" corresponds to res.min_multiplications = 5000 (not 4500) and the
example call matrix_chain_min_cost(dims) should return 5000; locate and change
the example usages of dims, res.min_multiplications, res.parenthesization, and
matrix_chain_min_cost in the comment block to these corrected values.
In `@parse_utils.H`:
- Around line 1601-1632: Both functions use std::vector<std::string>, which
violates the project policy for .H files—replace the STL container with the
Aleph equivalent (e.g., aleph::vector<std::string> or the project's-ah namespace
vector) in the signatures and local variables for load_file_lines and
split_string, update any necessary includes or using directives to pull in the
Aleph container header, and ensure return types and local pushes
(lines.push_back / result.push_back) continue to work with the Aleph container
API.
- Around line 1019-1023: The unconditional call to input_stream.unget() after
reading c can set the stream error state when c == EOF; update the termination
logic in the parsing routine that uses variables c, input_stream and
current_col_number so you only call input_stream.unget() when c != EOF (i.e., a
character was actually extracted), and decrement current_col_number only in that
same branch; apply the same conditional pattern to the other loaders that use
this termination idiom to avoid unget() on EOF.
- Around line 370-377: The branch in load_number that handles a '-' consumes a
next char and calls std::strtol on buffer without guaranteeing buffer is
null-terminated and without restoring the consumed non-digit to input_stream;
fix by ensuring the buffer is null-terminated before calling std::strtol (e.g.,
write a '\0' after the last written position in buffer) and restore the stream
when the char after '-' is not a digit (either unget/unread that char back into
input_stream or peek instead of consuming). Update the logic around
put_char_in_buffer, read_char_from_stream, and the '-' handling in load_number
so the buffer contains a valid C-string and the stream position is unchanged
when returning early.
In `@Tests/compiler_lexer_test.cc`:
- Around line 64-297: The tests don't cover the public Compiler_Lexer contract:
add unit tests that call Compiler_Lexer::source_file_id(), current_offset(),
eof(), and reset() (both valid offsets and an out-of-range offset to assert
rejection/diagnostic or exception), and verify peek/next behavior around resets
and EOF; specifically create a new TEST(CompilerLexer, PublicAPIContract) that
instantiates Compiler_Lexer, asserts source_file_id() matches the virtual file
id, advances with next() and checks current_offset() updates, verifies eof()
only after consuming End_Of_File, calls reset(valid_offset) and ensures
subsequent next()/peek() return tokens from that offset, and calls
reset(invalid_offset) to assert it triggers the expected error/diagnostic path
(e.g., Diagnostic_Engine or returned error).
In `@Tests/compiler_parser_test.cc`:
- Around line 75-77: The test performs a raw static_cast to Compiler_Let_Stmt
(e.g., the variable let_stmt created from fn->body->statements.access(0))
without first checking the node's runtime kind, which can cause undefined
behavior if the AST shape regresses; update the test to first read the base
pointer (e.g., auto *stmt = fn->body->statements.access(0)), assert stmt->kind
== Compiler_Stmt_Kind::Let (or the appropriate enum) and only then static_cast
to Compiler_Let_Stmt*, and apply the same guard-before-cast pattern to the other
occurrences mentioned (lines corresponding to the casts at 85-87, 122-124,
127-129, 174-176) to ensure safe, well-defined assertions.
In `@Tests/compiler_scope_test.cc`:
- Around line 46-120: Add a unit test that verifies the documented precondition
of Scope::leave_scope(): calling leave_scope() on an empty Scope must fail;
create a new TEST (e.g. "CompilerScope, LeaveOnEmptyScopeFails") that constructs
an empty Scope<std::string,int> and uses GoogleTest's death test macro
(EXPECT_DEATH or ASSERT_DEATH) to invoke scope.leave_scope() and assert the
process aborts (or otherwise fails) per the contract; reference the Scope class
and leave_scope() method so the test explicitly exercises that precondition.
In `@Tests/compiler_token_test.cc`:
- Around line 94-100: The test's EXPECT_EQ uses an incorrect expected format for
Compiler_Token::to_string; update the two expected strings to match the
documented contract "Kind: \"lexeme\"@span" used by Compiler_Token::to_string.
Specifically, change the Identifier expectation to use the format Identifier:
"value"@#1:[4,9) and change the String_Literal expectation to use
String_Literal: "\"hi\\n\""@#1:[10,16); adjust the EXPECT_EQ calls comparing
tok.to_string() accordingly so they assert the exact documented output.
- Around line 69-83: The test assertions in Tests/compiler_token_test.cc are
hard-coded and no longer match the actual mappings returned by
compiler_binary_precedence and compiler_binary_associativity; update the test to
assert against the authoritative API values (or derive expected values from the
same metadata) instead of stale literals: replace the fixed EXPECT_EQ calls for
Compiler_Token_Kind::Assign, ::OrOr, ::AndAnd, ::EqEq, ::LessEq, ::Plus, ::Star,
::Identifier and the associativity checks with checks that either call the
canonical metadata provider used by compiler_binary_precedence/
compiler_binary_associativity or use the current documented mapping so the test
exercises correctness of those functions (reference symbols:
compiler_binary_precedence, compiler_binary_associativity, Compiler_Token_Kind,
Compiler_Associativity).
- Around line 46-56: The test expectations don't match the inline
implementations in Compiler_Token.H: update the assertions so they reflect the
header's actual token classifications — change the
classify_compiler_keyword("return") expectation from
Compiler_Token_Kind::Kw_Return to the kind the header returns for the string
"return" (e.g., Compiler_Token_Kind::Identifier if the header doesn't define a
Kw_Return), and change the
compiler_token_is_literal(Compiler_Token_Kind::Kw_True) assertion to the correct
boolean per the header (switch to EXPECT_FALSE if Kw_True is not treated as a
literal, or assert the actual literal kind such as
Compiler_Token_Kind::Boolean_Literal if that exists); use the existing symbols
classify_compiler_keyword, compiler_token_is_literal, and
Compiler_Token_Kind::<...> to locate and update the two failing expectations.
In `@tpl_constraints.H`:
- Around line 302-347: apply() is repeatedly allocating duplicate composite
types because it never memoizes normalized results or canonicalizes newly
created tuples/functions; add a small memoization layer or canonicalization step
so equivalent shapes reuse the same id. Concretely: in
apply(Compiler_Type_Context &ctx, const Compiler_Type_Id id) maintain a local
(or persistent) map from input id to normalized id and return cached results
early (check after resolve_alias(id)), and when creating composites use/extend
Compiler_Type_Context to canonicalize instead of unconditionally calling
ctx.make_tuple_type(...) / ctx.make_function_type(...); e.g., check for an
existing interned tuple/function with the same members/result and return that
id, or have ctx.make_tuple_type/make_function_type perform canonicalization and
return the existing id if present. Ensure the memo map keys use the resolved id
and update it when you return resolved or newly canonicalized ids.
- Around line 526-605: unify() mutates the global substitution (subst) during
recursive unification and doesn't roll back those mutations when a deeper check
fails, leaving the solver in a polluted state; to fix this, make unify
transactional by snapshotting or recording any substitutions before attempting
to unify structured types (inside Compiler_Unify::unify and when calling
bind_variable/resolve), and if any recursive unify or arity/field check returns
a failure, restore the substitution state (revert the recorded bindings) and
return the failure (ensuring last is set to that failure); reference the unify
function, bind_variable, resolve and the subst data structure when locating
where to take a snapshot or track/undo bindings.
In `@tpl_scope.H`:
- Around line 56-241: The header adds a new public API class Scope<Key, Value>
but is missing required Doxygen for thread-safety and complexity; update the
class-level and every public method comment (Scope, Binding, enter_scope,
leave_scope, insert (both overloads), lookup_local (both), lookup (both), depth,
empty, clear) to state thread-safety guarantees (e.g., not thread-safe for
concurrent mutating ops; const lookup/depth/empty are safe for concurrent
readers only if external synchronization or mark as not thread-safe) and give
Big‑O complexity for each operation (enter_scope O(1), leave_scope O(1),
depth/empty O(1), clear O(total_bindings), insert O(k) where k is number of
bindings in current frame, lookup_local O(k), lookup O(total_bindings) or O(m*k)
describing frames m and per-frame bindings k). Ensure the comments also mention
exception behavior already used by current_frame()/leave_scope() (runtime error
on empty) so the Doxygen contract is complete.
---
Outside diff comments:
In `@scripts/planarity_certificate_validator.rb`:
- Around line 530-545: The resolve_executable method fails to find Windows
executables because it only treats paths containing File::SEPARATOR as
"absolute/relative" and, for bare names, never tries Windows executable
extensions; update resolve_executable to treat either File::SEPARATOR or
File::ALT_SEPARATOR (backslash) as indicating a path and, when searching PATH
for a bare command (e.g., "ruby"), try the common Windows extensions [".exe",
".bat", ".cmd", ".com"] by appending each extension to the candidate when the
plain candidate isn't an executable, returning the first matching File.file? &&
File.executable?; keep the existing behavior on non-Windows platforms.
---
Minor comments:
In `@Bellman_Ford.H`:
- Around line 49-57: The Doxygen example uses start without declaring it; add a
declaration (e.g., a vertex identifier compatible with List_Digraph<Node, Arc>)
before calling Bellman_Ford and has_negative_cycle so the snippet is
copy-pasteable — for example declare a variable named start of the appropriate
node/vertex type (matching List_Digraph<Node, Arc>) and initialize it to a valid
vertex in the constructed graph before calling bf.has_negative_cycle(start) and
bf.build_negative_cycle().
In `@Hungarian.H`:
- Around line 45-62: The example uses the wrong API and types: it calls
hungarian_min_assignment and constructs a DynArray<DynArray<double>>, but the
header actually exposes hungarian_assignment and expects a DynMatrix; update the
example to call hungarian_assignment and build/convert the input as a DynMatrix
(or show how to construct a DynMatrix from the nested arrays) and adjust the
expected result variable names (e.g., res.total_cost and res.row_to_col) to
match the actual return type of hungarian_assignment; ensure the example
compiles against the header by referencing hungarian_assignment and DynMatrix
explicitly.
In `@Knapsack.H`:
- Around line 45-64: Update the documentation for knapsack_01: correct the space
complexity row to O(n * capacity) because knapsack_01 uses a 2D DP table, and
fix the example result — for items Array<Knapsack_Item<size_t, int>> =
{{2,40},{3,50},{4,70},{5,80}} with capacity = 8 the correct optimal_value is 130
(items with weights 5 and 3), so update the commented optimal_value and
selected_items description accordingly.
In `@Matrix_Chain.H`:
- Around line 58-67: The example in the header uses unqualified function names
matrix_chain_order and matrix_chain_min_cost which can conflict; update the
example to call Aleph::matrix_chain_order and Aleph::matrix_chain_min_cost (or
add a using declaration explicitly) so the snippet is copy-paste safe and
unambiguous; change occurrences of matrix_chain_order(...) and
matrix_chain_min_cost(...) in the example to their qualified forms
Aleph::matrix_chain_order(...) and Aleph::matrix_chain_min_cost(...).
In `@README.md`:
- Around line 3961-3973: The API table uses the templated/internal name
Constraint_Set<T>, which is inconsistent with the exported/consumable symbol
naming used elsewhere; change the entry to the public concrete name
Compiler_Type_Constraint_Set (and if applicable mention Compiler_Type_Unifier
and Compiler_Type_Substitution remain) so the README lists the actual exported
type users should refer to instead of the internal template form; update the
table row under tpl_constraints.H to read Compiler_Type_Constraint_Set and keep
the rest of the description as-is.
In `@Tests/compiler_constraints_test.cc`:
- Around line 102-122: Add a failing-case to explicitly test solve()
short-circuiting: build a new Compiler_Type_Context, create type variables via
make_type_variable (e.g., A and B), then create a Compiler_Type_Constraint_Set
where the first constraint added is impossible (e.g., add(A, ctx.string_type(),
{}, "failing"), or a mismatched tuple) so solve(constraints) returns failure
immediately; after calling Compiler_Type_Unifier::solve verify result is not ok
and assert that constraints added after the failing one did not affect state by
checking unifier.apply on subsequent variables still yields their original
unconstrained types (use integer_type/string_type and symbols like add, solve,
apply to locate the code to change).
In `@Tests/planarity_test.cc`:
- Around line 681-683: Replace the throwing calls to fs::exists(candidate) and
fs::is_regular_file(candidate) with the non-throwing overloads that take a
std::error_code (or use fs::status(candidate, ec) + fs::is_regular_file(status))
so that permission errors don’t throw; specifically, update the checks around
the candidate path (variable candidate) to query existence and regular-file-ness
using an std::error_code ec (or a fs::file_status status obtained via
fs::status(candidate, ec)) and then test fs::exists(candidate, ec) or
fs::is_regular_file(status) and skip on errors instead of letting
fs::filesystem_error propagate.
---
Nitpick comments:
In `@Blossom.H`:
- Around line 43-47: Summary: The complexity table for Edmonds-Blossom in the
Doxygen header conflicts with the API note; make them consistent with the
implementation. Fix: update the table entry in Blossom.H (the Edmonds-Blossom
complexity block) to match the implementation and the API Doxygen note by
changing the time complexity to O(V^3) and the space complexity to O(V + E) (or
if the implementation truly uses O(V^2 * E), change the API note and any Doxygen
mention of O(V^3) to O(V^2 * E) and O(V) respectively); ensure both the table
and the API Doxygen block (the public API note referencing Edmonds-Blossom)
state the same complexities and reflect the actual algorithm complexity in the
codebase.
In `@Examples/compiler_sema_example.cc`:
- Around line 58-62: The code assumes parser.parse_module() always returns a
valid pointer; add a null-check on the variable module before calling
Compiler_Semantic_Analyzer::analyze_module to avoid dereferencing a null module.
Locate the call to parser.parse_module() and the subsequent use in
sema.analyze_module(module), and if module is null, log or report the parse
failure and skip or early-return from the demo instead of calling
sema.analyze_module.
In `@Examples/compiler_typed_sema_example.cc`:
- Around line 58-62: The example currently calls typed.analyze_module(module)
without checking whether parser.parse_module() returned nullptr; add a null
check for module after calling parse_module() and before constructing/using
Compiler_Typed_Semantic_Analyzer so we don't dereference a null pointer — e.g.,
if module is null, log or print an error and return/exit early instead of
calling Compiler_Typed_Semantic_Analyzer::analyze_module; refer to parse_module,
module, Compiler_Typed_Semantic_Analyzer, and analyze_module to locate the
relevant code.
In `@Tests/compiler_parser_test.cc`:
- Around line 44-178: Add a new unit test in Tests/compiler_parser_test.cc
(e.g., TEST(CompilerParser, ParsesEmptyInput)) that adds an empty virtual file
("" or whitespace) via Source_Manager::add_virtual_file, constructs
Diagnostic_Engine, Compiler_Ast_Context and Compiler_Parser (using same ctor
pattern), calls parser.parse_module(), and asserts the returned module is
non-null with zero functions and zero top-level statements and that
dx.has_errors() is false; place the test alongside the other CompilerParser
tests to cover the empty-source edge case.
In `@Tests/compiler_source_test.cc`:
- Around line 140-156: The test leaks the temp file if an assertion aborts
before fs::remove(path); wrap the temp-file lifetime in a cleanup guard (e.g., a
small RAII helper or scope guard) so the file created by
make_temp_file_path("source_manager") is removed in the guard's destructor;
update TEST SourceManager::LoadFileReadsContent to create the path then
construct the guard (tied to path) before writing/using std::ofstream out and
calling Source_Manager::load_file, ensuring cleanup always runs even on
ASSERT/EXPECT failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0984eedf-a570-4aa0-ad5b-c93f7819bad7
📒 Files selected for processing (62)
.clang-format.github/workflows/planarity_gephi_nightly.ymlBellman_Ford.HBlossom.HBlossom_Weighted.HCMakeLists.txtCompiler_AST.HCompiler_Lexer.HCompiler_Parser.HCompiler_Sema.HCompiler_Token.HCompiler_Typed_Sema.HCompiler_Types.HExamples/compiler_diagnostics_example.ccExamples/compiler_lexer_example.ccExamples/compiler_parser_example.ccExamples/compiler_sema_example.ccExamples/compiler_typed_sema_example.ccExamples/compiler_types_example.ccExamples/planarity_test_example.ccHungarian.HK_Shortest_Paths.HKnapsack.HLIS.HMatrix_Chain.HMin_Cost_Matching.HMin_Mean_Cycle.HPlanarity_Test.HREADME.es.mdREADME.mdSubset_Sum.HSuffix_Structures.HTests/CMakeLists.txtTests/compiler_ast_test.ccTests/compiler_constraints_test.ccTests/compiler_diagnostics_test.ccTests/compiler_lexer_test.ccTests/compiler_parser_test.ccTests/compiler_scope_test.ccTests/compiler_sema_test.ccTests/compiler_source_test.ccTests/compiler_token_test.ccTests/compiler_typed_sema_test.ccTests/compiler_types_test.ccTests/planarity_test.ccah-diagnostics.Hah-source.Hblossom_weighted_mwmatching.Hdocs/compiler_frontend_support.en.mddocs/compiler_frontend_support.mdhash-fct.Hmodular_arithmetic.Hmodular_linalg.Hparse_utils.Hpollard_rho.Hprimality.Hscripts/planarity_certificate_ci_batch.rbscripts/planarity_certificate_validator.rbscripts/planarity_gephi_templates.jsontikzgeom_scene.Htpl_constraints.Htpl_scope.H
👮 Files not reviewed due to content moderation or server errors (7)
- scripts/planarity_gephi_templates.json
- Compiler_Parser.H
- docs/compiler_frontend_support.md
- Compiler_Typed_Sema.H
- Compiler_Sema.H
- ah-diagnostics.H
- ah-source.H
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
Compiler_Lexer.H (1)
410-426:⚠️ Potential issue | 🔴 CriticalSingle-character operator token kinds also have naming mismatches.
Assignshould beEquals, andAmpshould beAmpersandbased on theCompiler_Token_Kindenum in the context snippet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Compiler_Lexer.H` around lines 410 - 426, The switch handling single-character operators uses incorrect enum names: change the token kind passed to make_token for '=' from Compiler_Token_Kind::Assign to Compiler_Token_Kind::Equals and for '&' from Compiler_Token_Kind::Amp to Compiler_Token_Kind::Ampersand so they match the Compiler_Token_Kind enum; locate the cases in the switch around the make_token calls (the '=' and '&' cases) and update the enum identifiers accordingly, leaving the make_token(begin, cursor) usage and other cases untouched.parse_utils.H (1)
1076-1079:⚠️ Potential issue | 🟠 MajorMust fix: these loaders still
unget()on EOF.
load_double()already guards the rewind, but these four loaders still use the old unconditional pattern. Inputs like0xFF,0755,0b1010, oridentifierat end-of-file can therefore leave the stream in an error state immediately after a successful parse.Proposed fix pattern
- input_stream.unget(); - if (c != '\n') - current_col_number--; + if (c != EOF) + { + input_stream.unget(); + if (c != '\n') + current_col_number--; + }Also applies to: 1122-1124, 1174-1176, 1228-1230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@parse_utils.H` around lines 1076 - 1079, Replace the unconditional input_stream.unget() and column decrement with the same EOF-guard pattern used in load_double(): only call input_stream.unget() and adjust current_col_number if the last-read character (c) is not EOF (or input_stream.good()/!input_stream.eof()), i.e., wrap the unget/decrement in a conditional that checks for EOF; apply this fix to every occurrence where you see input_stream.unget() followed by if (c != '\n') current_col_number-- so the four loader functions (the ones using that pattern, matching the snippet with variables input_stream, c, and current_col_number) no longer unget on EOF.
🧹 Nitpick comments (2)
Tests/compiler_lexer_test.cc (1)
64-113: Consider adding a test for empty input.The test suite doesn't explicitly verify behavior for an empty source file. Per coding guidelines, tests should cover edge cases including empty inputs.
💡 Suggested test case
TEST(CompilerLexer, EmptyInputReturnsEOFImmediately) { Source_Manager sm; const auto id = sm.add_virtual_file("empty.aw", ""); Compiler_Lexer lexer(sm, id); EXPECT_TRUE(lexer.eof()); const auto tok = lexer.next(); EXPECT_TRUE(tok.is_eof()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/compiler_lexer_test.cc` around lines 64 - 113, Add a new unit test in Tests/compiler_lexer_test.cc that verifies empty input returns EOF immediately: create a Source_Manager, call add_virtual_file with an empty string, construct a Compiler_Lexer with that file id, assert lexer.eof() is true, call lexer.next(), and assert the returned token .is_eof() is true (use the test name like CompilerLexer.EmptyInputReturnsEOFImmediately to match existing naming).Compiler_Parser.H (1)
279-299: Redundantexpect()call inparse_call_suffix.The
expect(LParen, ...)at line 282 will always succeed because the callerparse_postfix()(line 304) only callsparse_call_suffixwhenpeek().kind == Compiler_Token_Kind::LParen. Consider usingnext()directly instead to avoid the unnecessary check.♻️ Simplified version
Compiler_Expr *parse_call_suffix(Compiler_Expr *callee) { - const auto lparen - = expect(Compiler_Token_Kind::LParen, "expected '(' to start argument list", "PAR003"); + const auto lparen = next(); // Caller guarantees LParen auto *call = ast->make<Compiler_Call_Expr>(callee, lparen.span, Source_Span());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Compiler_Parser.H` around lines 279 - 299, The call to expect(Compiler_Token_Kind::LParen, ...) inside parse_call_suffix is redundant because parse_postfix only invokes parse_call_suffix when peek().kind == LParen; replace that expect with a direct token consumption (use next() to grab the '(' token) and assign it to lparen so you still have lparen.span for Compiler_Call_Expr and later rparen_span/merge; update parse_call_suffix to use next() for the LParen and keep the subsequent expect for the RParen unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Compiler_AST.H`:
- Around line 634-644: The ctor for Compiler_While_Stmt passes an undefined enum
symbol; replace the incorrect Compiler_While with the scoped enum value
Compiler_Stmt_Kind::While inside the base-class initializer (the call to
Compiler_Stmt(...)); update the initializer list in Compiler_While_Stmt to use
Compiler_Stmt_Kind::While so the type matches the enum used by Compiler_Stmt.
In `@parse_utils.H`:
- Around line 317-323: skip_white_spaces() currently calls
read_char_from_stream(input_stream) repeatedly and then unget()s the first
non-space char but does not adjust the column counter, leaving
current_col_number advanced by one; fix by decrementing current_col_number (the
same counter read_char_from_stream increments) before calling
input_stream.unget(), ensuring you guard against underflow (e.g., don't
decrement below zero) so token positions remain correct.
- Around line 1433-1445: The code currently writes a NUL from
process_escape('0') into the temporary C-string buffer and then returns
{buffer}, which uses C-string termination and truncates at NUL; change the token
finalization to use the actual written length (ptr - buffer_start) instead of
relying on a null-terminated C-string: update the close_token_scanning/return
path so the caller constructs the std::string from the buffer and length (e.g.,
std::string(buffer, len)) or have close_token_scanning return the length,
ensuring functions like put_char_in_buffer, process_escape,
read_char_from_stream remain unchanged but the final return uses the explicit
length to preserve embedded NUL bytes.
In `@scripts/planarity_certificate_validator.rb`:
- Around line 50-51: The built-in probe templates (notably
gephi_path_probe_template and the various smoke probe template strings that
currently hardcode "ruby -e") should be changed to invoke the current Ruby
interpreter via RbConfig.ruby instead of the literal "ruby" so the probe runs
with the same interpreter as the script; update each template string to
interpolate RbConfig.ruby in place of the hardcoded executable and run the same
probe logic. Also add/adjust a test to assert that an empty --gephi-cmd triggers
PATH probing (i.e., uses the probe template behavior) rather than falling back
to a hardcoded command.
---
Duplicate comments:
In `@Compiler_Lexer.H`:
- Around line 410-426: The switch handling single-character operators uses
incorrect enum names: change the token kind passed to make_token for '=' from
Compiler_Token_Kind::Assign to Compiler_Token_Kind::Equals and for '&' from
Compiler_Token_Kind::Amp to Compiler_Token_Kind::Ampersand so they match the
Compiler_Token_Kind enum; locate the cases in the switch around the make_token
calls (the '=' and '&' cases) and update the enum identifiers accordingly,
leaving the make_token(begin, cursor) usage and other cases untouched.
In `@parse_utils.H`:
- Around line 1076-1079: Replace the unconditional input_stream.unget() and
column decrement with the same EOF-guard pattern used in load_double(): only
call input_stream.unget() and adjust current_col_number if the last-read
character (c) is not EOF (or input_stream.good()/!input_stream.eof()), i.e.,
wrap the unget/decrement in a conditional that checks for EOF; apply this fix to
every occurrence where you see input_stream.unget() followed by if (c != '\n')
current_col_number-- so the four loader functions (the ones using that pattern,
matching the snippet with variables input_stream, c, and current_col_number) no
longer unget on EOF.
---
Nitpick comments:
In `@Compiler_Parser.H`:
- Around line 279-299: The call to expect(Compiler_Token_Kind::LParen, ...)
inside parse_call_suffix is redundant because parse_postfix only invokes
parse_call_suffix when peek().kind == LParen; replace that expect with a direct
token consumption (use next() to grab the '(' token) and assign it to lparen so
you still have lparen.span for Compiler_Call_Expr and later rparen_span/merge;
update parse_call_suffix to use next() for the LParen and keep the subsequent
expect for the RParen unchanged.
In `@Tests/compiler_lexer_test.cc`:
- Around line 64-113: Add a new unit test in Tests/compiler_lexer_test.cc that
verifies empty input returns EOF immediately: create a Source_Manager, call
add_virtual_file with an empty string, construct a Compiler_Lexer with that file
id, assert lexer.eof() is true, call lexer.next(), and assert the returned token
.is_eof() is true (use the test name like
CompilerLexer.EmptyInputReturnsEOFImmediately to match existing naming).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a713c4e2-bbc2-4e2d-b16d-19c4260bb4f8
📒 Files selected for processing (22)
.github/workflows/planarity_gephi_nightly.ymlBellman_Ford.HBlossom.HCompiler_AST.HCompiler_Lexer.HCompiler_Parser.HCompiler_Token.HExamples/compiler_sema_example.ccExamples/compiler_typed_sema_example.ccHungarian.HKnapsack.HMatrix_Chain.HREADME.mdTests/compiler_constraints_test.ccTests/compiler_lexer_test.ccTests/compiler_parser_test.ccTests/compiler_scope_test.ccTests/parse_utils_test.ccTests/planarity_test.ccparse_utils.Hscripts/planarity_certificate_validator.rbtpl_scope.H
✅ Files skipped from review due to trivial changes (8)
- Matrix_Chain.H
- Knapsack.H
- Tests/compiler_constraints_test.cc
- Blossom.H
- Hungarian.H
- Bellman_Ford.H
- Tests/planarity_test.cc
- tpl_scope.H
🚧 Files skipped from review as they are similar to previous changes (6)
- .github/workflows/planarity_gephi_nightly.yml
- Examples/compiler_sema_example.cc
- Tests/compiler_scope_test.cc
- Examples/compiler_typed_sema_example.cc
- Tests/compiler_parser_test.cc
- Compiler_Token.H
Refactor planarity certificate validator script to use RbConfig.ruby for better portability. Update compiler lexer test to include an empty input test case and refine existing API contract test. Adjust `Compiler_AST.H` to use `Compiler_Stmt_Kind` enum explicitly. Modify `parse_utils.H` to return `std::string` and fix minor doc formatting.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/planarity_certificate_validator.rb`:
- Around line 734-736: The sort block in smoke_templates.sort_by { |t|
-t.fetch("gephi_version", "0").to_s } applies unary minus to a String and
crashes; change the sort to use a proper version object instead of negating a
string—fetch the "gephi_version" from each template, wrap it with Gem::Version
(or parse into comparable components), sort by that comparable and then take the
highest (e.g. sort_by { |t| Gem::Version.new(t.fetch("gephi_version","0").to_s)
}.last or sort_by(...).reverse.first). Update the code around smoke_templates
and the "gephi_version" fetch to use Gem::Version so string negation is removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e04584fe-9e6a-4d20-aaf1-794ec8de9866
📒 Files selected for processing (4)
Compiler_AST.HTests/compiler_lexer_test.ccparse_utils.Hscripts/planarity_certificate_validator.rb
✅ Files skipped from review due to trivial changes (1)
- Compiler_AST.H
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/compiler_lexer_test.cc
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new compiler front-end support layer (source/spans, diagnostics, lexer/parser, scopes, name/type/typed-sema infrastructure) and updates surrounding docs/tests plus some unrelated maintenance (planarity Gephi probing, documentation enrichments, formatting config).
Changes:
- Introduces compiler-tooling headers (
ah-source.H,ah-diagnostics.H,Compiler_*,tpl_scope.H) and wires them into CMake, examples, and test targets. - Expands documentation/README coverage (EN+ES) and enriches multiple algorithm headers with complexity tables/examples/authorship.
- Adjusts planarity Gephi validation to probe executable availability via
PATH(templates, validator scripts, CI workflow, tests).
Reviewed changes
Copilot reviewed 63 out of 63 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tpl_scope.H | New generic nested-scope stack used by compiler name/type passes. |
| tikzgeom_scene.H | Adds Doxygen file header metadata. |
| scripts/planarity_gephi_templates.json | Switches Gephi “smoke” commands to PATH-probe via Ruby one-liner. |
| scripts/planarity_certificate_validator.rb | Implements PATH probing + default template selection for --gephi mode. |
| scripts/planarity_certificate_ci_batch.rb | Aligns Gephi default command behavior with validator changes. |
| primality.H | Adds author tag to Doxygen header. |
| pollard_rho.H | Adds author tag to Doxygen header. |
| modular_linalg.H | Adds author tag to Doxygen header. |
| modular_arithmetic.H | Adds author tag to Doxygen header. |
| hash-fct.H | Adds file-level Doxygen + minor signature constness tweak. |
| docs/compiler_frontend_support.md | New Spanish guide for compiler front-end support layer. |
| docs/compiler_frontend_support.en.md | New English guide for compiler front-end support layer. |
| blossom_weighted_mwmatching.H | Adds author metadata in header comment. |
| ah-source.H | New source manager/span/snippet utilities for compiler tooling. |
| ah-diagnostics.H | New deterministic plain-text diagnostics engine for compiler tooling. |
| Tests/planarity_test.cc | Updates Gephi availability check to PATH-based probe helper. |
| Tests/parse_utils_test.cc | Updates tests to use DynList instead of std::vector for returned lists. |
| Tests/compiler_types_test.cc | New unit tests for Compiler_Types.H. |
| Tests/compiler_typed_sema_test.cc | New unit tests for typed semantic analysis diagnostics/inference. |
| Tests/compiler_token_test.cc | New unit tests for token kinds/metadata helpers. |
| Tests/compiler_source_test.cc | New unit tests for Source_Manager behavior. |
| Tests/compiler_sema_test.cc | New unit tests for name resolution + semantic diagnostics. |
| Tests/compiler_scope_test.cc | New unit tests for Scope<Key,Value> behavior. |
| Tests/compiler_parser_test.cc | New unit tests for parser surface behavior and diagnostics. |
| Tests/compiler_lexer_test.cc | New unit tests for lexer tokenization and diagnostics. |
| Tests/compiler_diagnostics_test.cc | New unit tests for diagnostic accumulation/rendering determinism. |
| Tests/compiler_constraints_test.cc | New unit tests for constraint solving/unification outcomes. |
| Tests/compiler_ast_test.cc | New unit tests for arena-backed AST context/dump helpers. |
| Tests/CMakeLists.txt | Adds new compiler tests to the test program list. |
| Subset_Sum.H | Adds complexity table and usage example in docs. |
| README.md | Documents compiler tooling module + updates Gephi validation behavior/docs. |
| README.es.md | Spanish README updates mirroring README.md changes. |
| Planarity_Test.H | Adds author tag to Doxygen header. |
| Min_Mean_Cycle.H | Adds author tag to Doxygen header. |
| Min_Cost_Matching.H | Adds example + author metadata. |
| Matrix_Chain.H | Adds complexity table + example. |
| LIS.H | Improves Doxygen structure + adds complexity table + example. |
| Knapsack.H | Adds complexity table + example. |
| K_Shortest_Paths.H | Adds complexity table + example + author metadata. |
| Hungarian.H | Adds complexity table + example. |
| Examples/planarity_test_example.cc | Updates documented template id to portable.ruby-file-exists. |
| Examples/compiler_types_example.cc | New example demonstrating types + constraints/unification. |
| Examples/compiler_typed_sema_example.cc | New example demonstrating typed semantic analysis output/diagnostics. |
| Examples/compiler_sema_example.cc | New example demonstrating name-resolution + symbol dump + diagnostics. |
| Examples/compiler_parser_example.cc | New example demonstrating parsing + AST dump + diagnostics. |
| Examples/compiler_lexer_example.cc | New example demonstrating tokenization + token debug output. |
| Examples/compiler_diagnostics_example.cc | New example demonstrating spans + deterministic diagnostic rendering. |
| Compiler_Types.H | New stable type graph context for compiler MVP. |
| Compiler_Sema.H | New name-resolution + basic semantic checks layer. |
| Compiler_Parser.H | New recursive-descent parser for MVP grammar. |
| Compiler_Lexer.H | New single-file lexer producing stable-span tokens. |
| CMakeLists.txt | Registers new public headers in the build header list. |
| Blossom_Weighted.H | Adds author metadata in header comment. |
| Blossom.H | Updates Doxygen docs with complexity table + example. |
| Bellman_Ford.H | Reworks Doxygen docs (complexity table + example formatting). |
| .github/workflows/planarity_gephi_nightly.yml | Updates wording + shifts nightly check to executable-availability probe. |
| .clang-format | Enables BraceWrapping.AfterCaseLabel. |
Refactor header comments in Min_Cost_Matching.H for clarity and formatting. Update include statements and comments in subset_sum_test.cc.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds foundational compiler front-end infrastructure to Aleph-w (source/span management, deterministic diagnostics, parsing/semantic/type scaffolding) and integrates it into docs/examples/tests, while also making the planarity Gephi validation more portable via PATH-probing.
Changes:
- Introduces new compiler-support public headers (source manager, diagnostics engine, scope stack, lexer/parser, semantic and type infrastructure) and wires them into the build and test suite.
- Updates planarity certificate validation tooling/templates and docs to probe Gephi executable availability in
PATHby default (plus related CI/workflow adjustments). - Expands/normalizes documentation blocks across several existing headers (complexity tables/examples/author tags) and updates formatting rules.
Reviewed changes
Copilot reviewed 64 out of 64 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tpl_scope.H | Adds a generic nested-scope stack used by compiler name/type resolution. |
| tikzgeom_scene.H | Adds a Doxygen file header for the TikZ scene API. |
| scripts/planarity_gephi_templates.json | Updates Gephi “smoke” templates to PATH-probe via Ruby one-liners. |
| scripts/planarity_certificate_validator.rb | Shifts default --gephi behavior to PATH-probing, improves executable resolution, and updates built-in templates. |
| scripts/planarity_certificate_ci_batch.rb | Aligns CI batch runner defaults with new --gephi probing behavior. |
| primality.H | Adds author tag in Doxygen header. |
| pollard_rho.H | Adds author tag in Doxygen header. |
| modular_linalg.H | Adds author tag in Doxygen header. |
| modular_arithmetic.H | Adds author tag in Doxygen header. |
| hash-fct.H | Adds Doxygen file header; minor signature tweak for SuperFastHash. |
| docs/compiler_frontend_support.md | New Spanish guide documenting compiler front-end support scope and usage. |
| docs/compiler_frontend_support.en.md | New English guide documenting compiler front-end support scope and usage. |
| blossom_weighted_mwmatching.H | Adds author tag in Doxygen header. |
| ah-source.H | New source manager API for files, spans, positions, and snippets. |
| ah-diagnostics.H | New deterministic plain-text diagnostics engine built on Source_Manager. |
| Tests/subset_sum_test.cc | Formatting/structure cleanup in subset sum tests. |
| Tests/planarity_test.cc | Updates Gephi availability check to PATH-probe behavior. |
| Tests/parse_utils_test.cc | Updates tests to match DynList-based APIs (load_file_lines, split_string). |
| Tests/compiler_types_test.cc | Adds unit tests for the compiler type graph API. |
| Tests/compiler_typed_sema_test.cc | Adds unit tests for typed semantic analysis/type inference diagnostics. |
| Tests/compiler_token_test.cc | Adds unit tests for token classification/metadata and to_string(). |
| Tests/compiler_source_test.cc | Adds unit tests for Source_Manager behavior and edge cases. |
| Tests/compiler_sema_test.cc | Adds unit tests for name resolution, duplicates, shadowing, and control-flow misuse diagnostics. |
| Tests/compiler_scope_test.cc | Adds unit tests for Scope<Key, Value> shadowing and lifecycle. |
| Tests/compiler_parser_test.cc | Adds unit tests for parser behavior, options, and error recovery. |
| Tests/compiler_lexer_test.cc | Adds unit tests for lexer tokenization, options, and diagnostics. |
| Tests/compiler_diagnostics_test.cc | Adds unit tests for diagnostic building, rendering, determinism, and reset. |
| Tests/compiler_constraints_test.cc | Adds unit tests for constraint solving and unification edge cases. |
| Tests/compiler_ast_test.cc | Adds unit tests for arena-backed AST lifetime and deterministic dumping. |
| Tests/CMakeLists.txt | Registers new compiler-focused test targets. |
| Subset_Sum.H | Adds complexity table and example to documentation. |
| README.md | Documents compiler tooling headers and updates Gephi validation behavior/docs. |
| README.es.md | Spanish README updates mirroring Gephi validation and compiler tooling docs. |
| Planarity_Test.H | Adds author tag in Doxygen header. |
| Min_Mean_Cycle.H | Adds author tag in Doxygen header. |
| Matrix_Chain.H | Adds complexity table and example to documentation. |
| LIS.H | Adds complexity table and example to documentation. |
| Knapsack.H | Adds complexity table and example to documentation. |
| K_Shortest_Paths.H | Adds complexity table, example, and author tag to documentation. |
| Hungarian.H | Adds complexity table and example to documentation. |
| Examples/planarity_test_example.cc | Updates CLI examples to reference the new portable Ruby template id. |
| Examples/compiler_types_example.cc | New example demonstrating types + constraints/unification usage. |
| Examples/compiler_typed_sema_example.cc | New example showing parsing + typed semantic analysis + diagnostics. |
| Examples/compiler_sema_example.cc | New example showing parsing + name resolution + diagnostics. |
| Examples/compiler_parser_example.cc | New example showing parsing + AST dump. |
| Examples/compiler_lexer_example.cc | New example showing lexing + token debug output. |
| Examples/compiler_diagnostics_example.cc | New example showing source spans + deterministic diagnostics rendering. |
| Compiler_Types.H | New compiler type graph/context API. |
| Compiler_Sema.H | New semantic pass for name resolution and basic semantic checks. |
| Compiler_Parser.H | New recursive-descent parser for the MVP grammar. |
| Compiler_Lexer.H | New lexer for the MVP grammar built on Source_Manager. |
| CMakeLists.txt | Adds new compiler-support public headers to the installed header list. |
| Blossom_Weighted.H | Adds author tag in Doxygen header. |
| Blossom.H | Updates documentation with complexity table and example. |
| Bellman_Ford.H | Updates documentation formatting and complexity presentation. |
| .github/workflows/planarity_gephi_nightly.yml | Adjusts nightly job messaging and uses executable-availability probing. |
| .clang-format | Enforces brace wrapping after case labels. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
scripts/planarity_certificate_validator.rb (2)
712-739:⚠️ Potential issue | 🟠 MajorMust fix: keep the checked-in Gephi template catalog in sync with this new interpreter logic.
find_default_smoke_probe_templatenow auto-selects from the catalog before falling back toDEFAULT_GEPHI_TEMPLATES. The providedscripts/planarity_gephi_templates.json:1-30still hardcodesruby -e, so the default repo path still bypasses these newRbConfig.rubycommands. Please update the catalog too, or normalize those loaded entries here.Also applies to: 1346-1355
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/planarity_certificate_validator.rb` around lines 712 - 739, The new find_default_smoke_probe_template uses RbConfig to detect the Ruby interpreter and auto-selects templates loaded via load_gephi_templates, but the checked-in catalog (planarity_gephi_templates.json) still hardcodes "ruby -e" causing mismatches; update the catalog entries to use RbConfig.ruby-aware commands (or normalized forms) or normalize loaded entries in load_gephi_templates by replacing hardcoded "ruby -e" / "ruby" invocations with RbConfig::CONFIG['ruby_install_name']/RbConfig.ruby-based command strings so the template IDs and os matching work consistently with DEFAULT_GEPHI_TEMPLATES and the find_default_smoke_probe_template selection logic.
1087-1104:⚠️ Potential issue | 🟠 MajorShould fix: add regression coverage for the new empty-
--gephi-cmdflow.
parse_args,main, andvalidate_with_gephinow implement a different default--gephibehavior, but I don't see coverage for that branch in the provided context. A small CLI test around empty--gephi-cmd/--gephi-templatewould have caught the catalog-backed regression above.As per coding guidelines, "If the PR changes behavior, require tests or explain why they don't apply."
Also applies to: 1269-1291, 1346-1366
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/planarity_certificate_validator.rb` around lines 1087 - 1104, Add a regression test that exercises the new empty --gephi-cmd / --gephi-template code path to prevent the catalog-backed regression: write a small CLI-level test that calls parse_args (to construct args), invokes main (or the CLI entrypoint) with an explicit empty string for --gephi-cmd and appropriate --gephi-template flags, and asserts the behavior of validate_with_gephi (or the returned stats/errors/warnings) matches the intended new default (e.g., probes PATH, sets "gephi_command" to "path-probe <exe>", records "gephi_executable" and exit code 0, and produces the expected warnings/errors); add this test to the suite so changes to parse_args, main, or validate_with_gephi cannot regress this branch.
🧹 Nitpick comments (1)
scripts/planarity_certificate_validator.rb (1)
42-47: Nice to have: centralize the host-OS mapping.
default_gephi_executable_nameandfind_default_smoke_probe_templateboth decodeRbConfig::CONFIG["host_os"]independently. A shared helper would keep the fallback probe and the auto-selected template from drifting on less common platforms.Also applies to: 717-725
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/planarity_certificate_validator.rb` around lines 42 - 47, Centralize the host-OS decoding by extracting the RbConfig::CONFIG["host_os"] lookup and normalization into a single helper (e.g., normalized_host_os or host_platform) and use that helper from default_gephi_executable_name and find_default_smoke_probe_template (and other places noted around 717-725) instead of each method reading/normalizing host_os independently; update the logic in default_gephi_executable_name to call the new helper and keep the same platform checks (cygwin/mingw/mswin) so behavior is unchanged while avoiding duplicated decoding logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/planarity_certificate_validator.rb`:
- Around line 50-52: The command templates that interpolate RbConfig.ruby (e.g.,
in gephi_path_probe_template and the other templates at the same pattern) are
later tokenized with Shellwords.split and will break if the Ruby path contains
spaces; update each template to wrap RbConfig.ruby with Shellwords.escape so the
Ruby interpreter path is properly quoted/escaped (apply this change to the
occurrences matching the template pattern at the other affected sites as noted:
the similar interpolations at lines 61, 175, and 184), and ensure Shellwords is
required/imported where these templates are defined.
---
Duplicate comments:
In `@scripts/planarity_certificate_validator.rb`:
- Around line 712-739: The new find_default_smoke_probe_template uses RbConfig
to detect the Ruby interpreter and auto-selects templates loaded via
load_gephi_templates, but the checked-in catalog
(planarity_gephi_templates.json) still hardcodes "ruby -e" causing mismatches;
update the catalog entries to use RbConfig.ruby-aware commands (or normalized
forms) or normalize loaded entries in load_gephi_templates by replacing
hardcoded "ruby -e" / "ruby" invocations with
RbConfig::CONFIG['ruby_install_name']/RbConfig.ruby-based command strings so the
template IDs and os matching work consistently with DEFAULT_GEPHI_TEMPLATES and
the find_default_smoke_probe_template selection logic.
- Around line 1087-1104: Add a regression test that exercises the new empty
--gephi-cmd / --gephi-template code path to prevent the catalog-backed
regression: write a small CLI-level test that calls parse_args (to construct
args), invokes main (or the CLI entrypoint) with an explicit empty string for
--gephi-cmd and appropriate --gephi-template flags, and asserts the behavior of
validate_with_gephi (or the returned stats/errors/warnings) matches the intended
new default (e.g., probes PATH, sets "gephi_command" to "path-probe <exe>",
records "gephi_executable" and exit code 0, and produces the expected
warnings/errors); add this test to the suite so changes to parse_args, main, or
validate_with_gephi cannot regress this branch.
---
Nitpick comments:
In `@scripts/planarity_certificate_validator.rb`:
- Around line 42-47: Centralize the host-OS decoding by extracting the
RbConfig::CONFIG["host_os"] lookup and normalization into a single helper (e.g.,
normalized_host_os or host_platform) and use that helper from
default_gephi_executable_name and find_default_smoke_probe_template (and other
places noted around 717-725) instead of each method reading/normalizing host_os
independently; update the logic in default_gephi_executable_name to call the new
helper and keep the same platform checks (cygwin/mingw/mswin) so behavior is
unchanged while avoiding duplicated decoding logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5bd6868c-f010-431b-a0b2-f707a09b2419
📒 Files selected for processing (3)
Min_Cost_Matching.HTests/subset_sum_test.ccscripts/planarity_certificate_validator.rb
✅ Files skipped from review due to trivial changes (1)
- Tests/subset_sum_test.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- Min_Cost_Matching.H
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new compiler front-end support layer (source management, diagnostics, lexer/parser, scopes, sema, types) and makes Gephi validation/probing more portable across platforms while updating docs/tests/examples accordingly.
Changes:
- Introduces new compiler-support headers (e.g.,
ah-source.H,ah-diagnostics.H,tpl_scope.H,Compiler_{Lexer,Parser,Sema,Types}.H) plus accompanying unit tests and examples. - Updates Gephi/planarity validation templates and Ruby validator scripts to probe executable availability in
PATHrather than running--version, and improves Windows PATH resolution. - Expands documentation/README sections and adds/updates Doxygen headers and complexity/example blocks across multiple algorithm headers.
Reviewed changes
Copilot reviewed 64 out of 64 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tpl_scope.H | New generic nested-scope stack used by compiler semantic analysis. |
| tikzgeom_scene.H | Adds Doxygen file header metadata. |
| scripts/planarity_gephi_templates.json | Switches “smoke check” commands to PATH-availability probes via Ruby one-liners. |
| scripts/planarity_certificate_validator.rb | Adds platform detection, Ruby command normalization, PATH probing defaults, and improved Windows executable resolution. |
| scripts/planarity_certificate_ci_batch.rb | Adjusts default gephi_cmd behavior to align with validator’s new default probing. |
| primality.H | Adds author tag in Doxygen header. |
| pollard_rho.H | Adds author tag in Doxygen header. |
| modular_linalg.H | Adds author tag in Doxygen header. |
| modular_arithmetic.H | Adds author tag in Doxygen header. |
| hash-fct.H | Adds Doxygen file header; minor signature tweak for SuperFastHash. |
| docs/compiler_frontend_support.md | New Spanish guide for compiler front-end support layer. |
| docs/compiler_frontend_support.en.md | New English guide for compiler front-end support layer. |
| blossom_weighted_mwmatching.H | Adds # include <cstddef> (and author tag elsewhere) consistency. |
| ah-source.H | New source manager/spans/snippets infrastructure for compiler tooling. |
| ah-diagnostics.H | New deterministic plain-text diagnostic engine for compiler tooling. |
| Tests/subset_sum_test.cc | Formatting/organization tweaks in subset sum tests (no functional change intended). |
| Tests/planarity_test.cc | Makes Gephi availability check a PATH probe instead of running gephi --version. |
| Tests/parse_utils_test.cc | Replaces std::vector<string> expectations with DynList<string> iteration in tests. |
| Tests/compiler_types_test.cc | New unit tests for compiler type context behavior/printing. |
| Tests/compiler_typed_sema_test.cc | New unit tests for typed semantic analysis and diagnostic codes. |
| Tests/compiler_token_test.cc | New unit tests for token classification/metadata/to_string. |
| Tests/compiler_source_test.cc | New unit tests for source manager position/span/snippet behavior. |
| Tests/compiler_sema_test.cc | New unit tests for name resolution and semantic diagnostics. |
| Tests/compiler_scope_test.cc | New unit tests for Scope<Key, Value> behavior. |
| Tests/compiler_parser_test.cc | New unit tests for parser behavior, recovery, and options. |
| Tests/compiler_lexer_test.cc | New unit tests for lexer tokens/spans/comments/errors. |
| Tests/compiler_diagnostics_test.cc | New unit tests for diagnostic builder/rendering determinism. |
| Tests/compiler_constraints_test.cc | New unit tests for constraint solving/unification behavior. |
| Tests/compiler_ast_test.cc | New unit tests for arena-backed AST context reset and dump stability. |
| Tests/CMakeLists.txt | Registers new compiler-related test targets. |
| Subset_Sum.H | Adds complexity table and example block to docs. |
| README.md | Documents compiler tooling headers and updates Gephi adapter behavior/docs. |
| README.es.md | Spanish README updates mirroring compiler tooling + Gephi adapter changes. |
| Planarity_Test.H | Adds author tag in Doxygen header. |
| Min_Mean_Cycle.H | Adds author tag in Doxygen header. |
| Matrix_Chain.H | Adds complexity table and example block to docs. |
| LIS.H | Adds complexity table and example block to docs. |
| Knapsack.H | Adds complexity table and example block to docs. |
| K_Shortest_Paths.H | Adds complexity table, example block, and author tag. |
| Hungarian.H | Adds complexity table and example block to docs. |
| Examples/planarity_test_example.cc | Updates example commands to use portable.ruby-file-exists template. |
| Examples/compiler_types_example.cc | New example showcasing types + constraint solving. |
| Examples/compiler_typed_sema_example.cc | New example showcasing typed semantic analysis + diagnostics. |
| Examples/compiler_sema_example.cc | New example showcasing name resolution + diagnostics. |
| Examples/compiler_parser_example.cc | New example showcasing parsing + AST dumping. |
| Examples/compiler_lexer_example.cc | New example showcasing tokenization + token debug output. |
| Examples/compiler_diagnostics_example.cc | New example showcasing deterministic diagnostic rendering. |
| Compiler_Types.H | New stable type graph/type context for compiler tooling. |
| Compiler_Sema.H | New semantic pass for name resolution + basic semantic checks. |
| Compiler_Parser.H | New recursive-descent parser for MVP grammar. |
| Compiler_Lexer.H | New lexer built on Source_Manager, with optional diagnostics. |
| CMakeLists.txt | Adds new compiler-support public headers to the install/export list. |
| Blossom_Weighted.H | Adds author tag in Doxygen header. |
| Blossom.H | Adds complexity table + example documentation. |
| Bellman_Ford.H | Refactors header docs into a compact complexity table + example block. |
| .github/workflows/planarity_gephi_nightly.yml | Updates nightly Gephi workflow to validate downloaded executable availability (not --version). |
| .clang-format | Enforces brace wrapping after case labels. |
Comment on lines
+61
to
+63
| def gephi_path_probe_template(executable_name) | ||
| "#{Shellwords.escape(RbConfig.ruby)} -e \"name = ARGV.fetch(0); found = ENV.fetch('PATH', '').split(File::PATH_SEPARATOR).any? do |dir| path = File.join(dir, name); File.file?(path) and File.executable?(path) end; exit(found ? 0 : 1)\" #{executable_name}" | ||
| end |
Comment on lines
+48
to
+99
| # include <concepts> | ||
| # include <utility> | ||
|
|
||
| # include <ah-errors.H> | ||
| # include <tpl_dynArray.H> | ||
|
|
||
| namespace Aleph | ||
| { | ||
| /** @brief Stack of lexical scopes for bindings from `Key` to `Value`. | ||
| * | ||
| * Maintains a stack of frames, where each frame holds key-value bindings. | ||
| * Lookups search from the innermost (current) frame outward. | ||
| * | ||
| * @tparam Key Key type used for lookup. Must be equality-comparable. | ||
| * @tparam Value Stored binding payload. | ||
| * | ||
| * @note Thread-safety: Not thread-safe for concurrent mutating operations. | ||
| * Const operations (lookup, depth, empty) are safe for concurrent | ||
| * readers only if external synchronization is provided. | ||
| * @note Complexity: O(1) for construction. | ||
| */ | ||
| template <typename Key, typename Value> | ||
| requires std::equality_comparable<Key> | ||
| class Scope | ||
| { | ||
| public: | ||
| /** @brief Binding entry stored inside one lexical frame. | ||
| * | ||
| * @note Thread-safety: Not thread-safe. | ||
| * @note Complexity: O(1) storage and retrieval per binding. | ||
| */ | ||
| struct Binding | ||
| { | ||
| Key key; ///< Lookup key. | ||
| Value value; ///< Stored payload. | ||
| }; | ||
|
|
||
| private: | ||
| struct Frame | ||
| { | ||
| DynArray<Binding> bindings; | ||
| }; | ||
|
|
||
| DynArray<Frame> frames; | ||
|
|
||
| static Value * | ||
| find_value(DynArray<Binding> & bindings, const Key & key) noexcept | ||
| { | ||
| for (size_t i = bindings.size(); i > 0; --i) | ||
| if (bindings.access(i - 1).key == key) | ||
| return &bindings.access(i - 1).value; | ||
| return nullptr; |
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
Documentation
Tests
Bug Fixes
Chores