-
Notifications
You must be signed in to change notification settings - Fork 273
Clean out opensmt #3158
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
Clean out opensmt #3158
Conversation
kroening
commented
Oct 14, 2018
•
edited
Loading
edited
- Each commit message has a non-empty body, explaining why the change was made.
- My contribution is formatted in line with CODING_STANDARD.md.
- Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
- Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
- My commit message includes data points confirming performance improvements (if claimed).
- My PR is restricted to a single feature or bugfix.
- White-space or formatting changes outside the feature-related changed lines are in commits of their own.
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.
Passed Diffblue compatibility checks (cbmc commit: 014eea4).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/87877503
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.
Passed Diffblue compatibility checks (cbmc commit: 54cfd56).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/87881419
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.
How hard would it be to add tests for the other solvers? That would enable testing the change to arith_tools.cpp
for example?
The idea is that one day you can run all the tests in regression/cbmc with --cprover-smt2! We don't really need separate tests. |
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.
Can we please not do "to_integer now converts uninterpreted bitvectors"? That completely contradicts what the comments say what a bv_typet
is. I believe any interpretation of a bv_typet
as a signed or unsigned bitvector should go via a type cast, bit- or byte extract operation. Or we just kill bv_typet
completely and use unsignedbv_typet
instead.
src/cbmc/cbmc_parse_options.h
Outdated
@@ -46,7 +46,8 @@ class optionst; | |||
OPT_GOTO_CHECK \ | |||
"(no-assertions)(no-assumptions)" \ | |||
"(xml-ui)(xml-interface)(json-ui)" \ | |||
"(smt1)(smt2)(fpa)(cvc3)(cvc4)(boolector)(yices)(z3)(opensmt)(mathsat)" \ | |||
"(smt1)(smt2)(fpa)" \ | |||
"(cprover-smt2)(cvc3)(cvc4)(boolector)(yices)(z3)(mathsat)" \ |
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.
OpenSMT is still being developed by Hana and Natasha's teams; do we really want to remove support?
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.
Yes; it does not support any theories that we use.
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.
OK.
@smowton : systematic testing of the SMT solvers would be good but I think it might have to be as part of a larger campaign of "let's make SMT solvers the first choice for back-ends" and each time we've done the benchmarking on this ... it hasn't (yet) justified the effort. |
Support was dropped in April 2018.
54cfd56
to
145c895
Compare
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.
Given the claim that it no longer supports the necessary theories; remove seems sensible.
@tautschnig : This PR no longer includes the changes you objected to. |
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.
✔️
Passed Diffblue compatibility checks (cbmc commit: 145c895).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/90441669