Skip to content

Invoke callback for partial rule in chain. #273

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
hzhangxyz merged 1 commit into from
Mar 12, 2026
Merged

Invoke callback for partial rule in chain. #273

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 16 additions & 0 deletions src/chain.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ namespace ds {

length_t chain_t::execute(const std::function<bool(rule_t*)>& callback) {
std::set<std::unique_ptr<rule_t>, less_t> temp_facts;
std::set<std::unique_ptr<rule_t>, less_t> temp_rules;

bool break_all = false;

Expand All @@ -84,6 +85,21 @@ namespace ds {
break_all = true;
}
return;
} else {
do {
if (rule->data_size() > limit_size) {
break;
}
auto new_rule = std::unique_ptr<rule_t>(reinterpret_cast<rule_t*>(operator new(rule->data_size())));
memcpy(new_rule->head(), rule->head(), rule->data_size());
if (rules.find(new_rule) != rules.end() || temp_rules.find(new_rule) != temp_rules.end()) {
break;
}
temp_rules.emplace(std::move(new_rule));
if (callback(rule)) {
break_all = true;
}
Comment on lines +93 to +101

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block allocates/copies new_rule and stores it in temp_rules to deduplicate callbacks, but then invokes callback(rule) instead of using the stable copied instance. For intermediate rules, rule can point into the scratch buffer and may be overwritten later in the search, which is risky if callers retain the pointer. Prefer invoking the callback with the stored copy (or otherwise documenting/enforcing that the pointer is only valid during the callback).

Copilot uses AI. Check for mistakes.
Comment on lines +98 to +101

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New behavior: callbacks are now invoked for newly-generated partial rules (non-zero premises) and deduplicated via temp_rules. There are existing chain_t tests, but none assert that partial-rule callbacks fire (or fire only once) and that callback returning true stops the search promptly. Adding a focused unit test would help prevent regressions in this new observable behavior.

Copilot uses AI. Check for mistakes.
} while (false);
}
Comment on lines +99 to 103

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When callback(rule) returns true here, break_all is set but chain_recursive continues and still iterates over facts below (and can recurse further). This violates the documented “stop searching” behavior and differs from the premises_count()==0 branch which returns immediately. Consider returning immediately (and/or checking break_all before the facts loop / before recursing) once the callback requests termination.

Copilot uses AI. Check for mistakes.

for (auto& [fact, facts_cycle] : facts) {
Expand Down
Loading