Skip to content
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

Add wrappers for basic chuffed functions and example program using these wrappers #34

Merged
merged 4 commits into from
Nov 4, 2023

Conversation

Kieranoski702
Copy link
Contributor

@Kieranoski702 Kieranoski702 commented Nov 1, 2023

This pull request is mainly to commit my progress to the main branch. Everything compiles correctly but running main causing an address boundary error that needs to be solved.

I have based main.rs off vendor/examples/template.cpp as this was the simplest chuffed problem I could find. I have implemented the required rust wrappers in lib.rs to implement template.cpp in rust. I think that trying to re create a simple Chuffed problem before trying to implement the XYZ problem will make the process much simpler as I want to stick as close to chuffed as possible to minimize mistakes

My initial approach for figuring out what functions to implement in Rust was to look at the minizinc parser that Chuffed uses. The problem with this is that the entire parser is auto-generated and therefore extremely hard to follow. Instead, I decided that the example cpp files would be a good starting point as those should contain everything necessary to complete some problem.

My approach here is most likely flawed due to my limited knowledge of both cpp and rust so any suggestions to improve this before being merged are greatly appreciated!

Some notes about chuffed:

  • It uses global state to keep track of a problem which is quite different from something like minion I believe. This means that the Problem class found in each example cpp file is simply a way to organise the problem. The only thing a problem needs to implement is some way to print out/return the output variables.
  • Chuffed uses a lot of inheritance and classes so I have had to use a few ffi hacks inside wrapper.h/wrapper.cpp. For example, the branch function usually takes in a vec<Branching*>. The IntVar type, which is the main type for variables, inherits Branching so I had to create my own branch_IntVar function that takes in a vec<IntVar *> instead of `vec<Branching *>

@ChrisJefferson
Copy link
Contributor

I fixed the crash problem -- the issue is that createVars expects a vec<VarInt*>& -- Rust doesn't distinguish between * and & , and the two of them both act much like * in Rust. The difference in practice is that when you see a & , you have to pass an actual object of that type (vec* in this case), rather than a nullptr.

I added a method to create a vec<VarInt*>*, which we can then pass in. I've pushed the fix to your branch (which I actually didn't mean to do, woops!). I'm happy at some point (although it would have to be between 9am-11am due to time zones), to show how I debugged this problem, and fixed it, if you'd like more details, or hit similar issues.

@Kieranoski702
Copy link
Contributor Author

I fixed the crash problem -- the issue is that createVars expects a vec<VarInt*>& -- Rust doesn't distinguish between * and & , and the two of them both act much like * in Rust. The difference in practice is that when you see a & , you have to pass an actual object of that type (vec* in this case), rather than a nullptr.

I added a method to create a vec<VarInt*>*, which we can then pass in. I've pushed the fix to your branch (which I actually didn't mean to do, woops!). I'm happy at some point (although it would have to be between 9am-11am due to time zones), to show how I debugged this problem, and fixed it, if you'd like more details, or hit similar issues.

Thank you so much! I'd love to hop on a call at some point and go over how you debugged it just so I don't hit similar issues in the future. I see where I went wrong with the nullptr now though. Thanks again

@ozgurakgun
Copy link
Contributor

Are you both happy for this to be merged? Just to be clear we are happy to merge things as soon as they barely work at this stage, and use other PRs to improve them. So don't wait until it is absolutely ready before requesting a merge.

Maybe the threshold is having a test that tests whatever is being added. We should merge without at least one test case.

Soon when we have code coverage stats we can track this properly as well. Exciting.

@ChrisJefferson
Copy link
Contributor

The only change I would make before merging would be to copy some of the code currently in main into tests, just so there is some tests which execute the code, but then it can be merged, and improved / continued in another PR.

@ozgurakgun ozgurakgun mentioned this pull request Nov 4, 2023
@ozgurakgun ozgurakgun merged commit a74c59e into conjure-cp:main Nov 4, 2023
4 checks passed
@ozgurakgun
Copy link
Contributor

I merged this as is, can you please convert the main into a test in a separate PR @Kieranoski702?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants