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

Expose the full API for dynamic libraries and remove the need of using function pointers #139

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Nov 2, 2016

This is related to pull #79, only this time for 7.x and working demos :)

All static inline functions have been made inline with corresponding declarations in their translation units, as recommended by the C standard. As a side-effect, Chipmunk now compiles to shared libraries which have the full set of functions defined in the headers built into them, as opposed to only the function pointers in chipmunk_ffi.h.

Implementors of bindings to this library can still use the "old" way of using function pointers with cmake -DFUNCTION_POINTERS if they want to, but I recommend deprecating this approach and eventually removing it (unless I'm missing something). The way I did this is, is by moving chipmunk_ffi.h to a separate translation unit and adding it when the option is enabled in CMakeLists.txt.

Will be testing and using this in the Racket bindings.

@ghost ghost changed the title Expose the full API for dynamic libraries and remove the need for using function pointers Expose the full API for dynamic libraries and remove the need of using function pointers Nov 2, 2016
@ghost
Copy link
Author

ghost commented Nov 2, 2016

I was a bit too early, there's still something wrong with the generated shared lib. Will push as soon as I have figured out the problem.

@slembcke
Copy link
Owner

slembcke commented Nov 2, 2016

Yeah, was about to say I can't get it to link with Clang. It says all the functions are missing.

Also a heads up that it looks like you find/replaced "static" everywhere, including (non-function) things that were definitely supposed to be static, and some doc comments about "static bodies". ;)

My understanding of inline functions in C99 was that an inline function will hint to inline the function within it's own module only. Extern references to it would be regular function calls. Static inline functions get around that by making a private copy in each module, and skipping code generation if the function is unused. I didn't realize you could even put a non-inline function in a header without causing duplicate symbols, inline or not. I'm still not sure how it didn't cause duplicate symbol errors, and even more surprised that the linker can't find the functions at all.

So, my biggest worry is, have you run the benchmarks? With inlining disabled, Chipmunk is several times slower. Assuming the linking issue is resolvable, if the functions aren't inlined across modules then the performance is not going to be good.

Quite honestly, for most of the affected functions, I often recommend using a native type to implement them anyway. Do you really want to use cpfmin() instead of your language's generic min function? Even the vector functions are pretty trivial to reimplement in a hundred lines of code or so if you don't already have a preferred vector type.

@ghost
Copy link
Author

ghost commented Nov 3, 2016

Hi, thanks for your comments. One note, though: the functions still are inline, just not static anymore, so if I'm correct a decent compiler still should be able to make the optimisations. I might have communicated that wrongly, sorry. Will try to benchmark it in the upcoming days, I first need to fix a bug in one of my own related projects. And could you give an example of a function which should definitely not be static?

And agreed, some functions could be just as well implemented in the native language. Yet having the complete API makes it easier to do a full automated import of the library, and I'm not sure what the performance penalties are, given that calling C code is usually very fast.

Edit: and you got me there, definitely shouldn't make those variables non-static.

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.

1 participant