Skip to content

Do some TODOs #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
james-d-mitchell merged 5 commits into from
Dec 3, 2025
Merged

Do some TODOs #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6b880d5
don't check rules when adding
Joseph-Edwards Dec 2, 2025
ac8dfdf
Add always_reduced static const variable
Joseph-Edwards Dec 2, 2025
a75a599
Don't process pending rules after every overlap
Joseph-Edwards Dec 2, 2025
4d95139
Remove id from rules
Joseph-Edwards Dec 2, 2025
a9c7427
Add comments
Joseph-Edwards Dec 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions include/libsemigroups/detail/aho-corasick-impl.tpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,8 @@ namespace libsemigroups {
return *this;
}
}
// TODO Can this be improved so that we don't revisit suffixes that we
// have already checked?
while (_first != _last && _prefix != UNDEFINED) {
auto x = *_first;
++_first;
Expand Down
11 changes: 8 additions & 3 deletions include/libsemigroups/detail/knuth-bendix-impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -826,6 +826,8 @@ namespace libsemigroups {
detail::Ticker ticker;
// TODO bool old_ticker_running = _ticker_running;

// TODO add some way of accumulating the newly added rules, so we can
// reintroduce the stuff with the new rule trie.
while (_rules.number_of_pending_rules() != 0) {
while (_rules.number_of_pending_rules() != 0) {
Rule* new_rule = _rules.pop_pending_rule();
Expand Down Expand Up @@ -867,13 +869,16 @@ namespace libsemigroups {

// Reduce existing active rules wrt new_rule
void reduce_active_rules() {
// TODO remove RewriteFromLeft explicitly here
if constexpr (std::is_same_v<Rewriter, detail::RewriteFromLeft>) {
if constexpr (Rewriter::always_reduced) {
return;
} else {
auto const first = _rules.active_rules().begin();
auto const last = _rules.active_rules().end();

// For every lhs and rhs (<word>) of every active rule (<rule>), find
// every rule (<match>) whose lhs is a subword of <word>. If such a
// <match> exists, and it is not equal to <rule>, then the system is
// not reduced, so we make <rule> pending.
for (auto it = first; it != last; ++it) {
Rule* rule = *it;
for (auto const& word : {rule->lhs(), rule->rhs()}) {
Expand Down Expand Up @@ -987,7 +992,7 @@ namespace libsemigroups {

bool finished_impl() const override;
}; // class KnuthBendixImpl
} // namespace detail
} // namespace detail

////////////////////////////////////////////////////////////////////////
// global functions - to_human_readable_repr
Expand Down
38 changes: 25 additions & 13 deletions include/libsemigroups/detail/knuth-bendix-impl.tpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ namespace libsemigroups {
size_t operator()(detail::Rule const* AB,
detail::Rule const* BC,
typename native_word_type::const_iterator const& it) {
LIBSEMIGROUPS_ASSERT(AB->active() && BC->active());
LIBSEMIGROUPS_ASSERT(AB->state() == Rule::State::active
&& BC->state() == Rule::State::active);
LIBSEMIGROUPS_ASSERT(AB->lhs().cbegin() <= it);
LIBSEMIGROUPS_ASSERT(it < AB->lhs().cend());
// |A| + |BC|
Expand All @@ -47,7 +48,8 @@ namespace libsemigroups {
size_t operator()(detail::Rule const* AB,
detail::Rule const* BC,
typename native_word_type::const_iterator const& it) {
LIBSEMIGROUPS_ASSERT(AB->active() && BC->active());
LIBSEMIGROUPS_ASSERT(AB->state() == Rule::State::active
&& BC->state() == Rule::State::active);
LIBSEMIGROUPS_ASSERT(AB->lhs().cbegin() <= it);
LIBSEMIGROUPS_ASSERT(it < AB->lhs().cend());
(void) it;
Expand All @@ -62,7 +64,8 @@ namespace libsemigroups {
size_t operator()(detail::Rule const* AB,
detail::Rule const* BC,
typename native_word_type::const_iterator const& it) {
LIBSEMIGROUPS_ASSERT(AB->active() && BC->active());
LIBSEMIGROUPS_ASSERT(AB->state() == Rule::State::active
&& BC->state() == Rule::State::active);
LIBSEMIGROUPS_ASSERT(AB->lhs().cbegin() <= it);
LIBSEMIGROUPS_ASSERT(it < AB->lhs().cend());
(void) it;
Expand Down Expand Up @@ -590,12 +593,14 @@ namespace libsemigroups {
// understand, but it also didn't seem super important).
second = first++;
overlap(rule1, rule1);
while (second != _rules.active_rules().begin() && rule1->active()) {
while (second != _rules.active_rules().begin()
&& rule1->state() == Rule::State::active) {
--second;
detail::Rule const* rule2 = *second;
overlap(rule1, rule2);
++nr;
if (rule1->active() && rule2->active()) {
if (rule1->state() == Rule::State::active
&& rule2->state() == Rule::State::active) {
overlap(rule2, rule1);
++nr;
}
Expand Down Expand Up @@ -774,7 +779,7 @@ namespace libsemigroups {
if (p == q) {
return;
}
rules::add_pending_rule(_rules, p, q);
rules::add_pending_rule_no_checks(_rules, p, q);
}

//////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -826,15 +831,20 @@ namespace libsemigroups {
void
KnuthBendixImpl<Rewriter, ReductionOrder>::overlap(detail::Rule const* u,
detail::Rule const* v) {
LIBSEMIGROUPS_ASSERT(u->active() && v->active());
LIBSEMIGROUPS_ASSERT(u->state() == Rule::State::active
&& v->state() == Rule::State::active);
auto const &ulhs = u->lhs(), vlhs = v->lhs();
auto const &urhs = u->rhs(), vrhs = v->rhs();
auto const lower_limit = ulhs.cend() - std::min(ulhs.size(), vlhs.size());

int64_t const u_id = u->id(), v_id = v->id();
// TODO rm
// int64_t const u_id = u->id(), v_id = v->id();

for (auto it = ulhs.cend() - 1;
it > lower_limit && u_id == u->id() && v_id == v->id()
&& it < ulhs.cend() && !stop_running()
it > lower_limit &&
// TODO rm
// u_id == u->id() && v_id == v->id() &&
it < ulhs.cend() && !stop_running()
&& (_settings.max_overlap == POSITIVE_INFINITY
|| (*_overlap_measure)(u, v, it) <= _settings.max_overlap);
--it) {
Expand All @@ -849,9 +859,11 @@ namespace libsemigroups {
vlhs.cend()); // rule = AQ_j -> Q_iC
add_pending_rule(x, y);

if (_rules.number_of_pending_rules() >= _settings.max_pending_rules) {
process_pending_rules();
}
// TODO rm
// if (_rules.number_of_pending_rules() >=
// _settings.max_pending_rules) { process_pending_rules();
// }

// It can be that the iterator `it` is invalidated by the call to
// proccess_pending_rule (i.e. if `u` is deactivated, then
// rewritten, actually changed, and reactivated) and that is the
Expand Down
68 changes: 17 additions & 51 deletions include/libsemigroups/detail/rewriters.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,11 @@ namespace libsemigroups {
private:
native_word_type _lhs;
native_word_type _rhs;
int64_t _id; // TODO remove?
State _state;

public:
explicit Rule(int64_t id);
Rule() : _lhs(), _rhs(), _state(Rule::State::inactive) {}

Rule() = delete;
Rule& operator=(Rule const& copy) = delete;
Rule(Rule const& copy) = delete;
Rule(Rule&& copy) = delete;
Expand All @@ -83,35 +81,6 @@ namespace libsemigroups {
return _rhs;
}

// TODO rm
[[nodiscard]] bool empty() const noexcept {
return _lhs.empty() && _rhs.empty();
}

// TODO rm
[[nodiscard]] inline bool active() const noexcept {
LIBSEMIGROUPS_ASSERT(_id != 0);
return _id > 0;
}

// TODO rm
void activate_no_checks() noexcept;
// TODO rm
void deactivate_no_checks() noexcept;

// TODO rm
void set_id_no_checks(int64_t id) noexcept {
LIBSEMIGROUPS_ASSERT(id > 0);
LIBSEMIGROUPS_ASSERT(!active());
_id = -1 * id;
}

// TODO rm
[[nodiscard]] int64_t id() const noexcept {
LIBSEMIGROUPS_ASSERT(_id != 0);
return _id;
}

// TODO rm
void reorder() {
// if (ReductionOrder()(_lhs, _rhs)) {
Expand Down Expand Up @@ -247,7 +216,6 @@ namespace libsemigroups {
// TODO out of line
void add_inactive_rule(Rule* rule) {
rule->state(Rule::State::inactive);
rule->deactivate_no_checks(); // TODO rm
_inactive_rules.push_back(rule);
}

Expand Down Expand Up @@ -315,29 +283,23 @@ namespace libsemigroups {

namespace rules {

// TODO should be add_pending_rule_no_checks, and remove checks that lhs
// != rhs, assert instead.
// TODO to tpp
template <typename StringLike>
void add_pending_rule(Rules& rules,
StringLike const& lhs,
StringLike const& rhs) {
if (lhs != rhs) {
rules.add_pending_rule(
lhs.cbegin(), lhs.cend(), rhs.cbegin(), rhs.cend());
}
void add_pending_rule_no_checks(Rules& rules,
StringLike const& lhs,
StringLike const& rhs) {
LIBSEMIGROUPS_ASSERT(lhs != rhs);
rules.add_pending_rule(
lhs.cbegin(), lhs.cend(), rhs.cbegin(), rhs.cend());
}

// TODO should be add_pending_rule_no_checks, and remove checks that lhs
// != rhs, assert instead.
// TODO to cpp
inline void add_pending_rule(Rules& rules,
char const* lhs,
char const* rhs) {
if (lhs != rhs) {
rules.add_pending_rule(
lhs, lhs + std::strlen(lhs), rhs, rhs + std::strlen(rhs));
}
inline void add_pending_rule_no_check(Rules& rules,
char const* lhs,
char const* rhs) {
LIBSEMIGROUPS_ASSERT(lhs != rhs);
rules.add_pending_rule(
lhs, lhs + std::strlen(lhs), rhs, rhs + std::strlen(rhs));
}
} // namespace rules

Expand All @@ -364,6 +326,8 @@ namespace libsemigroups {
public:
using native_word_type = Rule::native_word_type;

static const bool always_reduced = false;

////////////////////////////////////////////////////////////////////////
// Constructors + inits
////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -454,6 +418,8 @@ namespace libsemigroups {
using native_word_type = Rule::native_word_type;
using iterator = Rules::iterator;

static const bool always_reduced = true;

RewriteFromLeft() = default;
RewriteFromLeft& init();

Expand Down
47 changes: 11 additions & 36 deletions src/detail/rewriters.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,24 +34,6 @@ namespace libsemigroups {
// Rule
////////////////////////////////////////////////////////////////////////

Rule::Rule(int64_t id) : _lhs(), _rhs(), _id(-1 * id) {
LIBSEMIGROUPS_ASSERT(_id < 0);
}

void Rule::activate_no_checks() noexcept {
LIBSEMIGROUPS_ASSERT(_id != 0);
LIBSEMIGROUPS_ASSERT(!active());
_id *= -1;
}

void Rule::deactivate_no_checks() noexcept {
LIBSEMIGROUPS_ASSERT(_id != 0);
// LIBSEMIGROUPS_ASSERT(active());
if (_id > 0) {
_id *= -1;
}
}

////////////////////////////////////////////////////////////////////////
// RuleLookup
////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -158,13 +140,12 @@ namespace libsemigroups {
Rule* rule;
if (!_inactive_rules.empty()) {
rule = _inactive_rules.front();
rule->set_id_no_checks(_stats.total_rules);
_inactive_rules.erase(_inactive_rules.begin());
} else {
// TODO could add x2 new Rules
rule = new Rule(_stats.total_rules);
rule = new Rule();
}
LIBSEMIGROUPS_ASSERT(!rule->active());
LIBSEMIGROUPS_ASSERT(rule->state() == Rule::State::inactive);
return rule;
}

Expand All @@ -189,7 +170,6 @@ namespace libsemigroups {
= std::min(_stats.min_length_lhs_rule, rule->lhs().size());

rule->state(Rule::State::active);
rule->activate_no_checks(); // TODO rm
_active_rules.push_back(rule);
for (auto& it : _cursors) {
if (it == _active_rules.end()) {
Expand All @@ -199,22 +179,17 @@ namespace libsemigroups {
}

void Rules::add_pending_rule(Rule* rule) {
if (rule->lhs() != rule->rhs()) {
rule->state(Rule::State::pending);
rule->reorder();
_pending_rules.push_back(rule);
_stats.max_pending_rules
= std::max(_stats.max_pending_rules, _pending_rules.size());
} else {
// TODO remove this clause and the return value?
add_inactive_rule(rule);
}
LIBSEMIGROUPS_ASSERT(rule->lhs() != rule->rhs());
rule->state(Rule::State::pending);
rule->reorder();
_pending_rules.push_back(rule);
_stats.max_pending_rules
= std::max(_stats.max_pending_rules, _pending_rules.size());
}

Rules::iterator Rules::make_active_rule_pending(iterator it) {
Rule* rule = *it;
LIBSEMIGROUPS_ASSERT(rule->state() == Rule::State::active);
rule->deactivate_no_checks();
add_pending_rule(rule);

if (it != _cursors[0] && it != _cursors[1]) {
Expand Down Expand Up @@ -618,7 +593,7 @@ namespace libsemigroups {

while (_rules->number_of_pending_rules() != 0) {
Rule* rule1 = _rules->pop_pending_rule();
LIBSEMIGROUPS_ASSERT(!rule1->active());
LIBSEMIGROUPS_ASSERT(rule1->state() == Rule::State::pending);
LIBSEMIGROUPS_ASSERT(rule1->lhs() != rule1->rhs());
// Rewrite both sides and reorder if necessary . . .
rewrite(rule1);
Expand Down Expand Up @@ -815,7 +790,7 @@ namespace libsemigroups {
bool rules_added_this_pass = false;
while (_rules->number_of_pending_rules() != 0) {
Rule* rule = _rules->pop_pending_rule();
LIBSEMIGROUPS_ASSERT(!rule->active());
LIBSEMIGROUPS_ASSERT(rule->state() == Rule::State::pending);
LIBSEMIGROUPS_ASSERT(rule->lhs() != rule->rhs());
// Rewrite both sides and reorder if necessary . . .
rewrite(rule);
Expand Down Expand Up @@ -929,7 +904,7 @@ namespace libsemigroups {
RewriteTrie::descendants_confluent(Rule const* rule1,
index_type current_node,
size_t overlap_length) const {
LIBSEMIGROUPS_ASSERT(rule1->active());
LIBSEMIGROUPS_ASSERT(rule1->state() == Rule::State::active);
if (_rule_trie.node_no_checks(current_node).terminal()) {
Rule const* rule2 = _rule_map.find(current_node)->second;
// Process overlap
Expand Down
Loading