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

docstring support for functions #396

Closed
wants to merge 1 commit into from

Conversation

melven
Copy link
Contributor

@melven melven commented Dec 20, 2023

Uses an additional (optional) argument in libcxxwrap-julia method functions to pass a docstring from C++ to Julia.

@melven
Copy link
Contributor Author

melven commented Dec 20, 2023

Hi,

thanks for the nice work on CxxWrap.jl.
I'm not so familiar with Julia but the pybind11 binding for Python allows to add docstrings which I find extremel useful to build usable interfaces from C++ for python.

So I tried to implement something similar, passing a string from C++ to Julia which is used as a docstring.
Works together with JuliaInterop/libcxxwrap-julia#140
Currently only works for method, I didn't figure out how this should be done for add_type.

I don't know the workflow for merging some changes into both CxxWrap and libcxxwrap-julia, so I just opened 2 pull-requests for further discussion.

Thanks for looking into this and happy holidays!
Melven

@barche
Copy link
Collaborator

barche commented Dec 31, 2023

Thanks for this and sorry for my slow response. This is definitely nice to have, I'll look into this over the next week and at the same time figure out a new way to test PRs that change both C++ and Julia sides, since currently this is too difficult.

@sloede
Copy link
Contributor

sloede commented Jan 2, 2024

Facing the question on how to properly add docstrings and came across this PR... very nice! Are there any updates on if/when this might make it to a released version of CxxWrap.jl?

@melven
Copy link
Contributor Author

melven commented Jan 8, 2024

[...] figure out a new way to test PRs that change both C++ and Julia sides, since currently this is too difficult.

At least for this case, it could work to update first libcxxwrap and then CxxWrap.jl: the only interface is the doc argument the CppFunctionInfoStruct:
After looking into the code for jl_new_struct, it seems to me that any additional arguments to jl_new_struct are just ignored (if the Julia struct has still fewer arguments).
The other way round (fewer arguments than needed) might not work (undefined behavior) -- the code in https://github.com/JuliaLang/julia/blob/b354ce7c41f0fd6caa8da115be38c53143a3d548/src/datatype.c#L1481 doesn't seem to check the number of va-args...

@barche
Copy link
Collaborator

barche commented Jan 11, 2024

I have pushed the changes here (plus the removal of the println to the testjll branch and merged the libcxxwrap-julia PR, so the CI there should now run with these changes. ONce this checks out I'll merge this manually.

@melven
Copy link
Contributor Author

melven commented Jan 12, 2024

Thanks!

@barche
Copy link
Collaborator

barche commented Jan 17, 2024

Merged manually into main, can already be used by adding CxxWrap#main

@barche barche closed this Jan 17, 2024
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.

3 participants