[prefactor] new engine_data and error ffi modules
#537
Conversation
github-actions
bot
added
the
breaking-change
| /// # Safety | ||
| /// `data_handle` must be a valid pointer to a kernel allocated `ExclusiveEngineData` | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn engine_data_length(data: &mut Handle<ExclusiveEngineData>) -> usize { |
There was a problem hiding this comment.
aside: This is a somewhat odd API... presumably engine provided this engine data instance to kernel in the first place, so presumably engine should already know how to get the length if it wants it?
There was a problem hiding this comment.
Actually... pretty much everything in this file feels weird/unsafe/incomplete (check out all those TODO comments). I guess eventually we'll need to clean all this up.
There was a problem hiding this comment.
yeah, this file is/was mostly a quick hack to allow the example program to actually read data. do note that if you're using the default engine this stuff is relevant and useful, but just that we haven't figured out all the ownership details quite yet
There was a problem hiding this comment.
yea I think this calls for a follow up issue
There was a problem hiding this comment.
feel free to chime in there: #538
ffi/src/error.rs
Outdated
| pub type AllocateErrorFn = | ||
| extern "C" fn(etype: KernelError, msg: KernelStringSlice) -> *mut EngineError; | ||
|
|
||
| // NOTE: We can't "just" impl From<DeltaResult<T>> because we require an error allocator. |
There was a problem hiding this comment.
This comment seems misplaced? Maybe it belongs with the IntoExternResult trait below?
There was a problem hiding this comment.
seems reasonable - moved!
ffi/src/lib.rs
Outdated
| pub mod engine_funcs; | ||
| pub mod error; | ||
| pub use error::*; |
There was a problem hiding this comment.
Is there a reason to bring all the error stuff into the root scope? It's a breaking change if we don't, but also, I think we do want things to live in error? It's also consistent with the kernel crate where we don't bring everything from error into the root.
There was a problem hiding this comment.
ok sounds good - I was thinking it would be 'better' to have this not change the APIs but I do agree it probably just makes sense to have them be exposed inside of mod error - ill do that!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #537 +/- ##
==========================================
- Coverage 81.04% 81.03% -0.01%
==========================================
Files 65 67 +2
Lines 14099 14099
Branches 14099 14099
==========================================
- Hits 11426 11425 -1
- Misses 2091 2093 +2
+ Partials 582 581 -1 ☔ View full report in Codecov by Sentry. |
(no-change code movement PR)
ffi/src/lib.rswas getting pretty long and included pieces from various things like error handling, etc. which are nicely-grouped as a standalone module. Secondly,ffi/src/scan.rsincluded some engine data utilities that could be moved out and likely shared in the future (nothing scan-specific). This PR just moves out some of these easily-grouped items into separate modules:mod engine_data: thepub fn's moved intomod engine_datawill need to be prefixed withengine_datanow to resolvemod error: i've donepub use error::*so error types will be exposed the same.