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

Add a form of dependant types #313

Closed
wants to merge 2 commits into from
Closed

Add a form of dependant types #313

wants to merge 2 commits into from

Conversation

gracjan
Copy link

@gracjan gracjan commented Jul 21, 2016

Allow for refinement of types based on other argument values. Useful in situations when size of arrays is known only by inspecting other elements. For example consider a callback called from C to JavaScript with this signature:

void (*write)(char *ptr, size_t size);

To handle it properly use this:

function cb_write_refine_ptr_type(ptr, size) {
    // here all non-refined values are present, refined ones are nulls
    return ref.refType(array(ref.types.uint8, size))
}

function cb_write(ptr, size) {
    // ptr is a Buffer of length size here
}

Now use the following Callback call:

ffi.Callback(ref.types.void, [
                    cb_write_refine_ptr_type, 
                    ref.types.size_t], cb_write);

@gracjan
Copy link
Author

gracjan commented Jul 21, 2016

Fixes #312.

I'd like to get an opinion if the design I proposed here is in general okay with the direction ffi is going.

@gracjan
Copy link
Author

gracjan commented Jul 21, 2016

There you go. Even tests are there. I do not know where to put documentation.

@TooTallNate
Copy link
Member

Thanks for the pull request. I'm having a hard time understanding why this is necessary. Why don't you just do the unwrapping in the cb_write()? This new function seems like unnecessary overhead so far.

As a side note, since you're dealing with raw bytes here, just do ptr = ref.reinterpret(ptr, size) in the first line of cb_write and you'll have a properly sized Buffer instance to work with.

@gracjan
Copy link
Author

gracjan commented Jul 21, 2016

Okay, this is unnecessary.

My confusion came from two places:

  1. I though that demarshaling was done inside _Callback and then buffers were copies of original buffers and were cut in my case.
  2. ref.reinterpret can extend beyond size of original buffer, I would not expect this to be the case as it is highly unsafe.

Might be good to put these two things somewhere in documentation. Thanks.

@gracjan gracjan closed this Jul 21, 2016
@TooTallNate
Copy link
Member

ref.reinterpret can extend beyond size of original buffer, I would not expect this to be the case as it is highly unsafe.

What do you mean by unsafe? Pretty much everything ffi-related is unsafe. You should see my failed efforts to get this module into node core 😞

@gracjan
Copy link
Author

gracjan commented Jul 21, 2016

Well, I know is unsafe. Just my experience with other buffer like structures was that they allow to take a slice of a buffer but do not allow to go outside of initial buffer length.

@TooTallNate
Copy link
Member

Just FYI, ref-array does a .reinterpret() under the hood: https://github.com/TooTallNate/ref-array/blob/cce5fb960c5ca05f9ba0928d02440fea0d4a76bc/lib/array.js#L261-L264, so the UInt8Array approach should be similarly unsafe 😉

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.

None yet

2 participants