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

Accept an options hash passed to Rubinius::FFI::Library#attach_function #3158

Merged
merged 3 commits into from
Oct 21, 2014
Merged

Accept an options hash passed to Rubinius::FFI::Library#attach_function #3158

merged 3 commits into from
Oct 21, 2014

Conversation

jemc
Copy link
Member

@jemc jemc commented Oct 5, 2014

This will make it easier for code targetting the FFI gem to use Rubinius::FFI when it is available by allowing keyword arguments to be passed to Rubinius::FFI::Library#attach_function. It will resolve issue #3074. An example of code using options passed to attach_function is here:
https://github.com/chuckremes/ffi-rzmq-core/blob/master/lib/ffi-rzmq-core/libzmq.rb#L62-L69

Currently, this patch accepts an options hash, but ignores all options.

The options used by the FFI gem are listed here, in the implementation:
https://github.com/ffi/ffi/blob/master/lib/ffi/library.rb#L214-L217

The :blocking option is a hint to the FFI gem that the C function may block and allows it to prepare MRI to release the GIL. It seems like this option can be safely ignored in Rubinius.
The :calling_convention option tells the FFI gem what calling convention to use. AFAIK, this is only to support windows API calls, and thus can safely be ignored in Rubinius.
The :enums and :type_map options are features that could be added to Rubinius::FFI, but in my opinion should be in a separate, more extensive patch.

I also cleaned up a few points of apparent cruft in the function implementation, including reindexing the argument names to make more sense and getting rid of a few assignments that were redundant or assigned to variables that were never used.

I also noticed that a Pointer is accepted as the c_name, so I documented that fact. However, there are currently no specs for this. I'll add some in a separate PR.

EDIT: It looks like using a Pointer as the c_name does not actually work, so it was just more cruft. The third commit in the PR now reflects this. If one tries to use a Pointer as the c_name, one gets the error:

TypeError: Tried to use non-reference value 0x12796 as type String (69)

@brixen brixen merged commit a73e911 into rubinius:master Oct 21, 2014
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