Skip to content

Conversation

@Joseph-Edwards
Copy link

This PR includes two changes:

  1. AhoCorasickImpl objects can now act as a map. Presently, this only works when the value type of the map is Rule *. The only reason for this was so I could quickly test everything was working as expected. I think it would be better if the value type was specified as a template parameter.
  2. The OverlapIteratorTrie object now exists as a way to iterate over the critical pairs stored in a trie. The returned type is an Overlap that contains pointers to the rules in the trie, and the overlap length.

Both KnuthBendix and RewritingSystemTrie now use these objects.

I have also included quite a few TODOs in this PR, many of which are phrased as questions which I expect can probably be answered quite quickly.

Things that should probably be done from here:

  • Make Overlap and AhoCorasickImpl template structs/classes that depend on the value type.
  • Decide what the API for AhoCorasickImpl should be moving forward. Should we be able to both insert and emplace? Do we still want to be able to query the data structure as if it were not a map? Should various members and functions be const or not?
  • Make better use of the trie where possible. In implementing the above, I tried to make the smallest number of changes so that the tests would pass. In most places, this looked like replacing _rule_map.find(x) with _rule_trie.nodes_no_checks(x).value(). However, it is possible that we can improve some things now that we don't have two data structures. The two places that come to mind are confluence checking and the SearchIterators. Confluence checking should probably be replaced by a very small call to that uses the OverlapIteratorTrie.

@james-d-mitchell james-d-mitchell merged commit 989eb6b into james-d-mitchell:improve-knuth-bendix-2 Dec 13, 2025
8 of 59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants