Repository navigation
Prepare release candidate - #15
Conversation
|
It seems better to leave the implementation of tell/told out of this repo, since it is too fundamental to belong in a wordnet library. But we need proper file stacking for the generation of the csv files. So maybe we should just use standard output predicates, rather than tell/told. |
ekaf
left a comment
There was a problem hiding this comment.
I tend to agree with this review by Claude Sonnet 4.6:
Review: Prepare release candidate (#15)
Recommendation: ✅ Approve and merge
Summary
This PR is well-scoped and ready to merge. It addresses three distinct improvements:
a bug fix, a performance enhancement, and licensing compliance — all with clear
documentation and verified output.
Notably, this PR has passed the multi-compiler CI workflow introduced by #17,
which runs the WordNet database validation (wn_valid.pl) against SWI-Prolog,
GNU Prolog, and Trealla Prolog. Passing this stricter automated test matrix
significantly increases confidence in the correctness and portability of the
changes.
Bug Fix — stacked_output_streams (closes #14)
The replacement of tell/told with open/close-based stream handling is the
correct portable fix. The previous approach could fail on Prolog systems that do
not support stacked output streams. The new code uses standard ISO stream
management and avoids unnecessary resource leaks.
Performance — Optimized Transitive Closure
The new closure_dyn/2 backend using a dynamic visited/1 predicate is a
welcome addition. The inline documentation clearly explains the trade-offs
between the two backends (O(E+V) dynamic vs. O(E·V + V²) ordset), and the
reference output confirms both algorithms produce identical results
(All closures size: 698873).
One caveat worth noting for future work: visited/1 is a global dynamic
predicate, so closure_dyn/2 is not thread-safe. This is acceptable for the
current use case but should be documented as a known limitation.
Licensing — SPDX Compliance
Adding machine-readable SPDX identifiers and restructuring the license
documentation into LICENSE.md + LICENSES/ is the correct REUSE-compliant
approach for a repository with dual-licensed content (Apache-2.0 for library
code, WordNet license for data files).
Checklist
- Bug fix resolves a reported issue (#14)
- New functionality is documented in source comments
- Reference output files updated and consistent
- Branch is up to date with base (
WNprolog-3.1) - Passes multi-compiler CI (SWI-Prolog, GNU Prolog, Trealla) introduced by #17
- No merge conflicts
Reviewed by GitHub Copilot
|
Using the Github Action from PR #18, releases are now produced like this: The resulting release is available here. |
Summary of Updates
This pull request prepares the repository for a new release and incorporates the following improvements:
Fix for
stacked_output_streams(close #14):Corrected the defective handling of stacked output streams. Previously, nested tell/told calls could fail on some Prolog systems. The fix ensures that streams are properly closed, reused, or garbage-collected as needed, preventing any unnecessary resource consumption.
Optimized Transitive Closure (dynamic visited-set backend):
Added an alternative transitive-closure backend that tracks visited nodes using a dynamic visited/1 predicate. On Prolog systems with well-indexed dynamic predicates (amortized O(1) lookup/assert), the closure traversal runs in O(E+V) for the reachable subgraph; result collection and cleanup add O(V), so overall remains O(E+V).
This is compared against the existing ordered-list visited-set approach based on ord_memberchk/2 + ord_insert/3 (from utils.pl portability code / ordsets). For list-based ordsets, membership and insertion are O(|Visited|) worst-case (with early cutoff due to ordering), giving worst-case O(E·V + V²) per closure, though it can still be faster in practice on some systems (notably SWI and Trealla) due to dynamic database overheads.
SPDX File Headers for Licensing Compliance:
Introduced machine-readable SPDX identifiers at the top of all source files to explicitly declare their licensing terms. This change clarifies downstream compliance requirements and aligns with modern practices for open-source repositories.