search

mishun / minisat-rust

Experimental minisat SAT solver reimplementation in Rust
Other
71 stars 6 forks source link

Update deps, format, use modern syntax in more cases, use naming conventions #6

Closed FauxFaux closed 6 years ago

FauxFaux commented 6 years ago

Fixes #3.

A whole bunch of changes, as separate commits, but as one pull request.

  1. Update deps, including breaking change in flate2.
  2. Run rustfmt, as that seems to be gaining adoption.
  3. Change try! to ? as that's stable everywhere (released Nov. 2016, in Debian and Ubuntu).
  4. Rename everything to snake case (#3).
  5. Fix a couple of warnings.
  6. Enable lto for cargo test --release.

The test doesn't pass for me before this change, but fails for the same reason after the change.

Feel free to pick and choose commits, instead of taking the whole thing. The rustfmt one is disruptive, obviously.

mishun commented 6 years ago

Thank you very much! I'm a bit busy right now but I'll definitely give it a look later this week.

May I ask what was the problem with tests? It seems that CI build has passed with no problem.

FauxFaux commented 6 years ago

The test is ignored, so the CI isn't running it: https://github.com/mishun/minisat-rust/blob/895dab548e57639a2972971fb2b46595b82f3419/tests/minisat.rs#L12

I assume I'm running a different version of minisat to what you developed against? I'm on minisat 1:2.2.1-5 from Ubuntu 17.10.

mishun commented 6 years ago

Yes, if I remember correctly, minisat from Ubuntu's repository is significantly behind minsat's current master branch. CI build runs tests with --ignored (idea with #[ignore] was to not wait >30min for each local cargo test)

FauxFaux commented 6 years ago

Oh, cool.

The version in Ubuntu is named the same as the upstream versions (from 2010).

The readme links: https://github.com/niklasso/minisat/releases http://minisat.se/MiniSat.html

...both of which haven't had any updates since 2010, and are what is packaged for Ubuntu.

Huh. Maybe I should work out what differs for me.

mishun commented 6 years ago

Unfortunately it is taking me much longer than I expected. I think I'll merge it as-is for now and maybe fix something later if it would be necessary. Thanks again!