-
Notifications
You must be signed in to change notification settings - Fork 150
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.
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
Fix match_with_constraint
#4499
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CTerm.constraint
seems to have two different uses.
b7c5bff
to
d5bcbd2
Compare
ec7162c
to
4fa4e82
Compare
Since #4628 determines the |
4fa4e82
to
298df8a
Compare
Downstream Validation: Other ReposNo direct use. CTerm.matchThe following check carries some risk because Lines 158 to 159 in 4fa4e82
Cterm.anti_unifyReturn a correct constraint. KCFG.create_coverReturn a correct constraint. KCFG.lift_split_edgeNeed Refactor KCFG.lift_split_splitNeed Refactor KCFG.merge_nodeNeed Simplify |
pyk/src/pyk/kcfg/minimize.py
Outdated
not_none(a.cterm.match_with_constraint(cterm)).add_constraint(constraint) | ||
for (cterm, constraint) in new_cterms_with_constraints | ||
] | ||
new_cterms_with_constraints = [csubst(a.cterm) for csubst in csubsts] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified by using create_split_by_nodes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename the variable to new_cterms
then, instead of new_cterms_with_constraints
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Stevengre I'm approving, but please see my suggestion for renaming a variable that is now simpler (new_cterms_with_constraints => new_cterms
perhaps?).
I think it's OK for now to wait for Petar to explain the need for that extra check, but let's leave it in for now. Once you have an example of where that check is hindering us from making progress, then it will be easier to add the appropriate test and weaken the check in a follow-up PR.
23b9fd2
to
e1eadcd
Compare
e1eadcd
to
123c835
Compare
Fixes #4496