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

fix UInt8 type #78

Closed
wants to merge 2 commits into from
Closed

fix UInt8 type #78

wants to merge 2 commits into from

Conversation

lmiq
Copy link
Contributor

@lmiq lmiq commented Nov 15, 2024

I was getting this error while reading some trajectories:

ERROR: MethodError: no method matching chfl_trajectory_with_format(::Ptr{UInt8}, ::Int8, ::Ptr{UInt8})

Closest candidates are:
  chfl_trajectory_with_format(::Ptr{UInt8}, ::UInt8, ::Ptr{UInt8})
   @ Chemfiles ~/.julia/packages/Chemfiles/Wzcgy/src/generated/cdef.jl:608

and this change seems to be the fix.

@lmiq
Copy link
Contributor Author

lmiq commented Nov 16, 2024

Note: I´m getting this error running Julia on a linux emulation inside an android device. The error only manifests on this platform for me.

@Luthaf
Copy link
Member

Luthaf commented Nov 18, 2024

I guess this is an issue with char being defined as either signed or unsigned on different platforms. A better fix might actually be to use Cchar, which seems to handle this for us.

@lmiq lmiq marked this pull request as draft November 18, 2024 11:50
@lmiq
Copy link
Contributor Author

lmiq commented Nov 18, 2024

thanks for the feedback. Changed it now, and tested in on both platforms (Linux emulated on ARM and x86) which I have access. Seems to work fine on both.

@lmiq lmiq marked this pull request as ready for review November 18, 2024 11:56
@Luthaf
Copy link
Member

Luthaf commented Nov 18, 2024

I might need to update the CI for this repository it is still using some old setup. I'll cherry-pick your commit on top after that.

@lmiq
Copy link
Contributor Author

lmiq commented Nov 18, 2024

Don't worry if just changing the line is easier that merging the PR. I really don't mind. Thanks for your support.

@Luthaf
Copy link
Member

Luthaf commented Nov 18, 2024

PR is replaced by #80

@Luthaf Luthaf closed this Nov 18, 2024
@Luthaf
Copy link
Member

Luthaf commented Nov 27, 2024

The fix is now released in version 0.10.42!

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