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

Error on invalid LAMMPS instance #62

Merged
merged 5 commits into from
Aug 7, 2024
Merged

Error on invalid LAMMPS instance #62

merged 5 commits into from
Aug 7, 2024

Conversation

Joroks
Copy link
Collaborator

@Joroks Joroks commented Aug 6, 2024

I've taken a few things from #34 and made sure that we check the lammps instance before using it for all of our public API

@Joroks Joroks requested a review from vchuravy August 6, 2024 09:59
@vchuravy
Copy link
Member

vchuravy commented Aug 6, 2024

Ah interesting, so the issue with checking before an operation is that the error is "far" away from the original location.
Once should always check after so that the error is thrown close to the offending operation.
It's probably sensible to check before as well to make sure we don't pass an invalid state.

@Joroks
Copy link
Collaborator Author

Joroks commented Aug 6, 2024

The main issue I'm adressing here is that it's not valid to pass null pointers as the handle for a lammps instance (with very few exceptions). Doing this currently causes a crash. I've decided to include this check in the check method for simplicity. We could also seperate that out though.

@vchuravy
Copy link
Member

vchuravy commented Aug 6, 2024

Actually we can define next to our unsafe_convert function:

Base.cconvert(::Type{Ptr{Cvoid}}, lmp::LMP) = lmp.lmp == C_NULL ? error("") : lmp

So that whenever a LMP instance is passed to a C-API we emit an error check.

src/LAMMPS.jl Outdated Show resolved Hide resolved
src/LAMMPS.jl Outdated Show resolved Hide resolved
src/LAMMPS.jl Outdated Show resolved Hide resolved
@Joroks Joroks merged commit c838e7e into cesmix-mit:main Aug 7, 2024
14 of 16 checks passed
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