-
Notifications
You must be signed in to change notification settings - Fork 0
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
Initialize basic tests #2
Conversation
Drvi
commented
Dec 12, 2023
- Set up CI
- Simplify FFI a bit
container::Cstring | ||
key::Cstring | ||
host::Cstring | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you say why this is preferable to the existing code?
It looks more complex to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of manually casting struct between two types, we rely on the Julia machinery that is supposed to solve this problem: the cconvert
that transforms the data into C-compatible types and followed by the unsafe_convert
which can then safely create pointers from the whatever it was given by cconvert
. Those two are automatically called by @ccall
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is how you are supposed to do this thing, because it handles lifetimes for you and is applicable to all @ccall
s we'll do for list
, head
and others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if you are really against it, I'll remove it
@@ -0,0 +1,45 @@ | |||
name: CompatHelper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this worth having even if we have no deps
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just copied everything over, I thought it might be useful eventually, but can remove it if you want to
I'll merge it now, but I'm happy to make adjustments later |