-
Notifications
You must be signed in to change notification settings - Fork 23
refactor(pumpkin-solver): Use the fzn-rs package instead of flatzinc
#258
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: main
Are you sure you want to change the base?
Conversation
…he solve item in TypedInstance
945b37e to
438aa3b
Compare
898a593 to
244b353
Compare
The Rc is not Send, but popular crates like anyhow do expect errors to be Send. Since these are errors, we pay the price to allocate a String instead.
… fzn-rs" This reverts commit 8c5b945.
Review re-requested
|
let mut typed_ast |
cfe2685 to
1ee9455
Compare
ImkoMarijnissen
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.
Only went through the changes in pumpkin-solver
| } | ||
|
|
||
| ast::Domain::UnboundedInt => { | ||
| return Err(FlatZincError::UnsupportedVariable(name.as_ref().into())) |
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.
We now "support" this
| // A literal which is always true, can be used when using bool constants in the solver | ||
| // pub(crate) constant_bool_true: BooleanDomainId, | ||
| // A literal which is always false, can be used when using bool constants in the solver | ||
| // pub(crate) constant_bool_false: BooleanDomainId, |
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.
This commented out code can be removed
| instance | ||
| .resolve_array(array) | ||
| .map_err(|err| FlatZincError::UndefinedArray(err.0))? | ||
| .map(|maybe_int| maybe_int.map_err(FlatZincError::from)) |
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.
To me this is a bit vague; why could this be an int?
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.
Other compiler files have some documentation using //! that specifies the overall purpose of the file; would be nice to include that here as well
| instance: &Instance, | ||
| context: &mut CompilationContext, | ||
| ) -> Result<(), FlatZincError> { | ||
| for (name, array) in &instance.arrays { |
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.
Is this something that we previously did not support or did we just handle it in a different way previously?
| ast::Domain::UnboundedInt => { | ||
| return Err(FlatZincError::UnsupportedVariable(name.as_ref().into())) |
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.
Not unsupported anymore
1ee9455 to
7d97aaf
Compare
We migrate to the
fzn-rspackage, which extracts much of the parsing of flatzinc files to a separate crate.Todo
fzn-rs, a better FlatZinc parser that is easier to re-use across projects #238main