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

Copy Ceed Object References #739

Merged
merged 3 commits into from
Apr 15, 2021
Merged

Copy Ceed Object References #739

merged 3 commits into from
Apr 15, 2021

Conversation

jeremylt
Copy link
Member

This adds reference copying to libCEED. This creates a second, separate reference to the same Ceed object that can (and should) be independently destroyed.

@knepley, I think this will address your segfault. @YohannDudouit, this might also be of interest for MFEM.

@jeremylt
Copy link
Member Author

Note: I can't reproduce the CI issue locally yet. I'm investigating - it might be a cargo_tarpaulin issue?

@jeremylt
Copy link
Member Author

This crate isn't building correctly in CI: https://github.com/KyleMayes/clang-sys
but I'm not sure why. It doesn't have anything to do with us though

@jeremylt jeremylt force-pushed the jeremy/copy-ptr branch 3 times, most recently from c96b2aa to aaca7c7 Compare April 12, 2021 15:53
@jeremylt
Copy link
Member Author

I don't know how I fixed it, but I fixed CI. Ready to merge.

Copy link
Member

@jedbrown jedbrown left a comment

Choose a reason for hiding this comment

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

All looks fine. Would using "reference" as a verb, as in CeedReference(Ceed ceed), with CeedReferenceCopy(Ceed src, Ceed *dest) or CeedReferenceShare(Ceed src, Ceed *dest) be better for autocomplete?
PETSc does not have a "copy" function like this, but it could be used most places where we currently spell out the operations.

@jeremylt jeremylt force-pushed the jeremy/copy-ptr branch 2 times, most recently from e1542db to 485a5fd Compare April 12, 2021 18:52
@jeremylt
Copy link
Member Author

I updated the function names. A fix is in for rust-lang/rust on what I think the Rust issue is, but I don't know when it will merge. We should be ok to just merge whenever you're ready @jedbrown

Copy link
Contributor

@YohannDudouit YohannDudouit left a comment

Choose a reason for hiding this comment

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

@jeremylt Was there a specific reason for you to think this could be useful in MFEM?
Otherwise this looks good to me.

@jeremylt
Copy link
Member Author

I don't have a current specific use case in mind - it just seems like something that might be useful in the future

@jeremylt
Copy link
Member Author

Rust is happy, but GitHub is confused. This is good but no green button for some reason

@jedbrown jedbrown merged commit b997b43 into main Apr 15, 2021
@jedbrown jedbrown deleted the jeremy/copy-ptr branch April 15, 2021 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants