-
Notifications
You must be signed in to change notification settings - Fork 87
feat[ffi]: add non-panicking accessors to ffi #4333
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
base: develop
Are you sure you want to change the base?
Conversation
Signed-off-by: Will Manning <[email protected]>
Signed-off-by: Will Manning <[email protected]>
Signed-off-by: Will Manning <[email protected]>
Unable to generate the performance reportThere was an internal error while processing the run's data. We're working on fixing the issue. Feel free to contact us on Discord or at [email protected] if the issue persists. |
Codecov Report❌ Patch coverage is
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Will Manning <[email protected]>
Signed-off-by: Will Manning <[email protected]>
Signed-off-by: Will Manning <[email protected]>
Signed-off-by: Will Manning <[email protected]>
Signed-off-by: Will Manning <[email protected]>
Signed-off-by: Will Manning <[email protected]>
| array: *const vx_array, | ||
| start: u32, | ||
| stop: u32, | ||
| // TODO(aduffy): deprecate this from the FFI API. |
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.
lol, I just saw this comment @a10y -- I started this PR because the C example will just segfault on panic, which is not very easy to debug 😬
| /// vx_try_shutdown_runtime(); // Only succeeds if no sessions active | ||
| /// ``` | ||
| #[unsafe(no_mangle)] | ||
| pub extern "C" fn vx_try_shutdown_runtime() { |
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 can't imagine we actually want to commit this to as a stable API...?
We should make it clear (in another PR) the state of this API. Honestly, I'd rather just kill it for now and publish language-specific bindings that are allowed to make lock-step breaking changes to their own C APIs.
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.
oh yeah, this is gross. it's just that not cleaning up the runtime can lead to segfaults on exit when e.g., used with mimalloc. we could also kill this function and have the runtime be cleaned up when the last VortexSession is dropped
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.
Not that I disagree but this is how the api would look like in C anyway? You're sahying we shouldn't have C api and only C++?
Summary
Transforms the Vortex FFI layer from a panic-prone interface to a robust, safe-by-default API by replacing all panicking accessor functions with error-handling variants.
Key Changes
🛡️ Core Safety Transformation
vx_array_get_*functions now includevx_error **error_outparameter📖 Enhanced Documentation
🧪 Robust Testing
Impact
✅ Security
✅ Usability
✅ Performance
Breaking Changes
All primitive accessor functions now require an additional
vx_error **error_outparameter:Files Changed
vortex-ffi/src/array.rs: Core implementation with safe accessorsvortex-ffi/cinclude/vortex.h: Auto-generated header with new signaturesvortex-ffi/examples/hello-vortex.c: Updated example demonstrating proper usageResult
Production-ready FFI interface with comprehensive error handling, memory safety guarantees, and extensive test coverage. The API transformation ensures that unsafe usage patterns are caught at compile-time, preventing runtime crashes.