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

Automate the wrapper generation, upgrade CEnum dep. #77

Closed
rschwarz opened this issue Jan 4, 2019 · 9 comments
Closed

Automate the wrapper generation, upgrade CEnum dep. #77

rschwarz opened this issue Jan 4, 2019 · 9 comments
Milestone

Comments

@rschwarz
Copy link
Collaborator

rschwarz commented Jan 4, 2019

This applies only after #76 is merged.

We now use Clang.jl to generate Julia wrappers from the SCIP header files automatically.
There is a script for that purpose that should be rerun for every (major) release of SCIP that we want to support.

Unfortunately, the process was not completely automatic. The steps should be documented clearly, or integrated properly in the generator script.

In particular, the following steps followed the execution of the script:

  • rename Ptr{SCIP} to Ptr{SCIP_}, to avoid name conflict with module.
  • delete empty wrappers for type_*.h; the type definitions all went to commons.jl
  • add some missing (type/enum) definitions to manual_commons.jl (why did Clang.jl not add them?)

Also, we used the branch of Clang.jl from PR #210, which was now merged to master, but not yet tagged?

@rschwarz
Copy link
Collaborator Author

rschwarz commented Jan 4, 2019

Actually, the new Clang.jl seems to be tagged now (at 0.8).

Other potential improvements:

  • Automatically generate the wrapper.jl file (with all the include() statements).
  • Copy documentation strings from SCIP's headers to our Julia functions.
  • Use Ref{Ptr{}} instead of Ptr{Ptr{}}?
  • Use a stricter function signature for our ccall wrappers.

@bhalonen
Copy link

@rschwarz
Copy link
Collaborator Author

This is also based on Clang.jl, so it would be mostly for convenience of configuration?

@bhalonen
Copy link

yeah, my friend (the writer of the package) tried it on your package and (using the PR JuliaLang/julia#32658, I think) and auto wrapped everything but 3 inline functions. So it would add functionality by wrapping the varadic stuff you have as well.

@rschwarz
Copy link
Collaborator Author

OK, good to know.

Well, the variadic functions that are used do actually work now, there is just a risk of segfault or other unexpected behavior when targeting a new platform (or C compiler?).

I would have a look in the future, in case a re-generation of the wrapper becomes necessary after a release of a new SCIP version or similar. Right now, it works well enough for me not to bother, but PRs are welcome!

@rschwarz
Copy link
Collaborator Author

Another topic that has come up on Discourse. The auto-generated ccall wrappers all use Ptr{X} for some SCIP struct X. This is fine in most cases, as we will actually pass C pointers (for SCIP-owned objects) to these calls.

However, there are also cases where we have Julia-owned Julia objects, that we want to pass to SCIP because they are used as data associated with constraints etc. The correct way of specifying the ccall signature would be to use the Any type. Then, the Julia object would be automatically cconverted into a pointer and GC-safe (for the duration of the ccall).

I'm not sure how to handle that in the wrapper generator, since there might not be a simple pattern to distinguish these two cases. In fact, some SCIP functions might be useful to call both ways (passing C data, or passing Julia data).

@rschwarz rschwarz changed the title Document and automate the wrapper generation process. Automate the wrapper generation, upgrade CEnum dep. Jun 22, 2020
@Gnimuc
Copy link
Contributor

Gnimuc commented Mar 3, 2021

This may be fixed by #186, but I need more info.

rename Ptr{SCIP} to Ptr{SCIP_}, to avoid name conflict with module.

#186 generates a new module LibSCIP, so one can use LibSCIP.SCIP for distinguishing.

delete empty wrappers for type_*.h; the type definitions all went to commons.jl

Could you elaborate more about what is an empty wrapper?

add some missing (type/enum) definitions to manual_commons.jl (why did Clang.jl not add them?)

Clang.jl's new generator now works in a strict mode, if some symbols are missing, it just errors out and complains about which symbol is not found. Then, one needs to either add the missing header(if it's a dependent header but not living in the source tree) or use the new @add_def sym(if it's a symbol defined in std headers or system headers). As long as the generation runs successfully, there shouldn't be missing definitions except in the case of macro definitions.

Copy documentation strings from SCIP's headers to our Julia functions.

This can be easily supported in the new generator by adding a documenter pass.

Use Ref{Ptr{}} instead of Ptr{Ptr{}}?

As discussed in JuliaInterop/Clang.jl#120, there is really no much difference between these two versions, but I can add a new option to let users make the decision.

Use a stricter function signature for our ccall wrappers.

I think Clang.jl's old generator had an option for this, but Julia's function should be loosely typed. Why do you want to catch the error at the wrapper function instead of ccall? If you're concerned about mistakenly passing opaque pointers, which may crash Julia, you could generate mutable struct opaque_type end instead of const opaque_type = Cvoid by setting opaque_as_mutable_struct = true in the generator config toml file.

@rschwarz
Copy link
Collaborator Author

rschwarz commented Mar 3, 2021

Thanks for your review and comments.

Could you elaborate more about what is an empty wrapper?

Some of the header files in SCIP only contain struct definitions, or even typedefs. I found that the actual type definitions were all moved to commons.jl and that Clang.jl created Julia files corresponding to the C headers that only contained comments and empty lines. So, it's just a cosmetic problem.

I think Clang.jl's old generator had an option for this, but Julia's function should be loosely typed. Why do you want to catch the error at the wrapper function instead of ccall?

To be honest, I don't remember the reason for that bullet point. Maybe it was about adding a form of documentation.

If the wrapper-generation is fully automatic (it looks that way in your PR #186) then I would be very happy to merge the changes, even without taking any of the "improvements" into account.

@matbesancon
Copy link
Member

closed by #186

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

No branches or pull requests

4 participants