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

Remove mutability requirement on input from public facing API #18

Merged

Conversation

oisyn
Copy link
Contributor

@oisyn oisyn commented Oct 10, 2023

The new Rust version seems to have extra checks on casting &T to &mut T. The Rust-facing API just copies METIS' C API, which accepts pointers to non-const, which is why those casts were needed. But now I'm getting all these errors I figured why not fix the API itself.

I went ahead and made all input parameters immutable. In the unsafe blocks where the C API is called is then dealt with casting the *const T to *mut T.

@oisyn
Copy link
Contributor Author

oisyn commented Oct 10, 2023

Wait why did 1.61.0 suddenly start failing? Cargo.lock hasn't changed since the last commit.

EDIT: oh I see why, Cargo.lock is specified in .gitignore, so it downloads the latest version of all crates that fit within the spec of Cargo.toml. That's unfortunate. This basically means that checks can fail at any moment.

UPDATE: A fix for this is made in #20. This will resolve the failing tests for this PR.

@oisyn oisyn mentioned this pull request Oct 11, 2023
@hhirtz
Copy link
Member

hhirtz commented Oct 11, 2023

Thanks. Can you also update /examples/graph.rs and ensure the C numbering is used? We can hide this option then.

Having fortran numbering makes metis mutate xadj and adjncy:
https://github.com/KarypisLab/METIS/blob/e0f1b88b8efcb24ffa0ec55eabb78fbe61e58ae7/libmetis/pmetis.c#L116-L120
https://github.com/KarypisLab/METIS/blob/e0f1b88b8efcb24ffa0ec55eabb78fbe61e58ae7/libmetis/fortran.c#L16-L28
https://github.com/KarypisLab/METIS/blob/e0f1b88b8efcb24ffa0ec55eabb78fbe61e58ae7/libmetis/options.c#L87

@oisyn
Copy link
Contributor Author

oisyn commented Oct 11, 2023

Oh good call, I hadn't noticed that. Yeah, let's remove the Fortran numbering, but what do we do with set_options? It basically allows a user to bypass the exposed options.

We could either check on set_options and panic, or do the check in one of the calls that do the actual work and then return a proper error.

@hhirtz
Copy link
Member

hhirtz commented Oct 11, 2023

Hmm. If we go down the "checks" route as planned in #11 then might as well make set_options return a Result.

@oisyn
Copy link
Contributor Author

oisyn commented Oct 11, 2023

Alright, I've placed a panic! in set_options for now. We can return a proper error as part of the work in #11.

Copy link
Member

@hhirtz hhirtz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@hhirtz hhirtz merged commit 1aaf678 into LIHPC-Computational-Geometry:master Oct 11, 2023
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.

2 participants