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

@gsl_function macros #128

Closed
wants to merge 1 commit into from
Closed

@gsl_function macros #128

wants to merge 1 commit into from

Conversation

lxvm
Copy link
Contributor

@lxvm lxvm commented Dec 29, 2023

Hi,

I have been writing an extension for GSL.jl in Integrals.jl and I encountered #110. Following this post on discourse I rewrote @gsl_function to work with runtime closures, however I am not sure I am handling the memory correctly. For example, do I have to use GC.@preserve as written here? Or can the function_ field of the gsl_function struct become a Base.CFunction?

The relevant documentation also makes it clear that a cfunction closure is not supported on all platforms, which makes this pr less desirable.

The above being said, if/when a solution is clear the following would need to be done

  • rewrite the 5 other gsl_function macros with the same solution

@simonbyrne
Copy link
Member

Since GSL lets you pass an arbitrary thunk parameter (see https://julialang.org/blog/2013/05/callback/), I would suggest doing something similar to what we do in HDF5.jl:
https://github.com/JuliaIO/HDF5.jl/blob/91ef2841dcc5ddd52ef3094120b8921e0c1c5749/src/api/helpers.jl#L76-L86
Define a single "helper" cfunction which unwraps the thunk and evaluates the function, and pass the function itself via the thunk. This would avoid the runtime closure issue completely.

@simonbyrne simonbyrne mentioned this pull request Dec 30, 2023
@lxvm
Copy link
Contributor Author

lxvm commented Dec 30, 2023

Thank you for the guidance and links @simonbyrne! Due to the advantages you describe, the ease of mirroring a C idiom, and since my PhD adviser is the author of the blog post, I am in favor of the solution in your pr.

@lxvm lxvm closed this Dec 30, 2023
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