-
Notifications
You must be signed in to change notification settings - Fork 966
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
Initial C API for FFI #663
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The c_api.h header should be set up for use by consuming applications as well as the draco library itself.
Additionally, draco's C-API should not export all symbols, it should only export the symbols required for the C API
src/draco/c_api/c_api.h
Outdated
#if defined(_MSC_VER) | ||
#define EXPORT_API __declspec(dllexport) | ||
#else | ||
// Other platforms don't need this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GCC and Clang do need an EXPORT_API definition since GCC v4
It is strongly recommended not to export all functions, especially for a C-API as it greatly reduces the chance of name collisions and speeds up the dylib/so loader.
The draco C-API so/dylib should be compiled with the flag -fvisibility=hidden, and the following added for GCC and clang compilers:
#ifdef DRACO_BUILDING_DLL // also update draco_targets.cmake
#define EXPORT_API __attribute__ ((visibility ("default")))
#else
#define EXPORT_API
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little bit tricky to implement with the current approach of building the C-API as part of the "core" library, but I do agree it should happen.
The problem I'm facing if only the C-API is exported then the executables (draco_decoder
and draco_encoder
) nor the tests can be correctly linked as those depend on the C++ API.
I'm starting to lean towards having a separate library for the C-API, as the CMake orchestration would be much more easy to manage and the DRACO_C_API
will be orthogonal to other features. What do you think @RichardTea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's reasonable to pick an API for your shared libraries at configure time, as I'm reasonably sure that most consumers of the C-API will not want to use the C++ version of the dynamic library anyway.
The whole point of a C-API is to ensure a consistent ABI, so exposing all the C++-mangled stuff could rather complicate things on some compilers.
If it's difficult to build all four at the same time then I think draco_encoder, draco_decoder and the C-API shared lib could all consume the same C++ static library "privately" to achieve the goal of hiding all the internal symbols - IIRC, symbols aren't ever truly private in a static library so that should still work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've finally moved the C-API to its own library called cdraco.[dll/lib/so/a/...]
, gated by the DRACO_C_API
flag. This library is self contained and the dynamic version just exports the C-API, all the C++ stuff is hidden.
I'm adding a new optional pre-processor definition: |
@qmuntal: Thank you for the preliminary design and patches for adding a C-API to Draco. At this time the Draco team does not have the resources to accept a C-API. We are not currently able to review, test, or maintain the additional API and glue code necessary for a fully functional C-API. If a C-API is a critical requirement for your purposes, at least for the time being, you'll need to maintain it in a fork of the Draco repository, or in another project that depends on the Draco repository. Again, thank you for the PR, but we cannot accept it into the project at present. I will close this PR for now. If the situation changes the Draco team will revisit this subject. |
@RichardTea @tomfinegan I completely understand the situation and appreciate the efforts you have put on this reviews. I'll probably keep this changes in a fork in order to support at least the Go wrapper I plan to implement (https://github.com/qmuntal/go-draco). And of course I'll be more than happy to help on revamping this PR if your team have enough bandwidth to maintain it. |
Goal
This PR contains a proposal of C API to facilitate the interoperability with languages other than C++.
It does not try to define and implement an API for all the C++ API, just a minimal decoding subset, as there are a lot of design decisions that need to be made before going for the full implementation.
Context
imports other C++ headers from the
src
tree. Distributed in a separate library.src
tree. Distributed in a separate library.Design Goals
Decision points
DRACO_C_API
cmake flag. Disabled by default.void*
)void *
removes all semblance of type safetyExample of FFI
I´ve implemented a Go wrapper using the C API provided in this PR to demonstrate its capabilities. You can find it in https://github.com/qmuntal/go-draco/blob/master/draco.go.
Next steps