-
Notifications
You must be signed in to change notification settings - Fork 54
Add generic equality and distinct to the API #559
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
base: master
Are you sure you want to change the base?
Conversation
kfriedberger
left a comment
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.
Overall, a very good idea.
Please check for some special cases, as well as the visitor for a resulting formula, if not yet done somewhere.
|
|
||
| assertThat(f).isEqualTo(g); | ||
| } | ||
|
|
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.
Please add futher tests, e.g. for:
- zero items as parameters
- one item as parameters
- visitor of the resulting expression for several theories.
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.
I've now added a few more tests:
- zero and one parameters are covered by
testWrongNumberArgs - visitors are tested by
testEqualityVisitorandtestDistinctVisitor
| return equal(ImmutableList.copyOf(pArgs)); | ||
| } | ||
|
|
||
| BooleanFormula equal(List<Formula> pArgs); |
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.
Someone implementing this interface will need to adjust their implementation. We could provide a default implementation throwing UnsupportedOperationException if not overridden.
I am unsure if we ever did it that way in JavaSMT. Maybe we just added new methods as it is done here. Then we can continue with that strategy, i.e., keep the PR as-is. 😄
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.
Hmm.. I think I'd prefer to keep it unimplemented. The compiler errors were quite helpful in figuring out what still needs to be updated. If we add a default implementation that throws UnsupportedOperationException users would have to read the changelog to find out that new methods were added
Hello everyone,
in SMTLIB
=anddistinctare defined for all sorts and with a variable number of argument:We already support equality, but it's a different method for each
FormulaManager, and only two arguments can be used. Distinct is only supported for bitvectors and numeral formulas, but missing from the other formula managers. While it's possible to simulatedistinctby rewriting the formula, this leads to quadratically many terms, which may not always be desirable.This PR tries to fix the issue by adding two generic methods for
equalityanddistinctto theFormulaManager. The changes to the API are backwards compatible, and I've included a default implementation ofdistinctand=with multiple arguments for solvers that don't support these operations natively