Jefffrey commented on code in PR #10431:
URL: https://github.com/apache/arrow-rs/pull/10431#discussion_r3747417728
##########
arrow-array/src/ffi_stream.rs:
##########
@@ -97,17 +97,24 @@ const ENOSYS: i32 = 38;
#[derive(Debug)]
#[allow(non_camel_case_types)]
pub struct FFI_ArrowArrayStream {
+ // Fields are private so safe code can't install a bogus callback that
import
+ // or [`Drop`] would invoke. Write the release fields with the unsafe
setters.
Review Comment:
```suggestion
// Fields are intentionally private so safety guarantees can be upheld
via
// explicit unsafe functions
```
##########
arrow-array/src/ffi_stream.rs:
##########
@@ -213,6 +220,48 @@ impl FFI_ArrowArrayStream {
private_data: std::ptr::null_mut(),
}
}
+
+ /// Returns the producer-provided release callback, if any.
+ ///
+ /// Lets a consumer wrap release: save this callback, install its own with
+ /// [`FFI_ArrowArrayStream::set_release`], and chain back to it on drop.
See
+ /// <https://github.com/apache/arrow-rs/issues/9771>.
+ pub fn release(&self) -> Option<unsafe extern "C" fn(arg1: *mut Self)> {
Review Comment:
```suggestion
pub fn release(&self) -> Option<unsafe extern "C" fn(arg1: *mut Self)> {
```
preferable to just have this detail on the setter
##########
arrow-array/src/ffi_stream.rs:
##########
@@ -97,17 +97,24 @@ const ENOSYS: i32 = 38;
#[derive(Debug)]
#[allow(non_camel_case_types)]
pub struct FFI_ArrowArrayStream {
+ // Fields are private so safe code can't install a bogus callback that
import
+ // or [`Drop`] would invoke. Write the release fields with the unsafe
setters.
/// C function to get schema from the stream
- pub get_schema:
- Option<unsafe extern "C" fn(arg1: *mut Self, out: *mut
FFI_ArrowSchema) -> c_int>,
+ get_schema: Option<unsafe extern "C" fn(arg1: *mut Self, out: *mut
FFI_ArrowSchema) -> c_int>,
/// C function to get next array from the stream
- pub get_next: Option<unsafe extern "C" fn(arg1: *mut Self, out: *mut
FFI_ArrowArray) -> c_int>,
+ get_next: Option<unsafe extern "C" fn(arg1: *mut Self, out: *mut
FFI_ArrowArray) -> c_int>,
/// C function to get the error from last operation on the stream
- pub get_last_error: Option<unsafe extern "C" fn(arg1: *mut Self) -> *const
c_char>,
+ get_last_error: Option<unsafe extern "C" fn(arg1: *mut Self) -> *const
c_char>,
/// C function to release the stream
- pub release: Option<unsafe extern "C" fn(arg1: *mut Self)>,
- /// Private data used by the stream
- pub private_data: *mut c_void,
+ ///
+ /// Private so safe code can't install a callback that [`Drop`] would
invoke.
+ /// Use [`FFI_ArrowArrayStream::release`] and
[`FFI_ArrowArrayStream::set_release`].
+ release: Option<unsafe extern "C" fn(arg1: *mut Self)>,
+ /// Private data used by the stream, owned by the release callback.
+ ///
+ /// Private for the same reason as `release`. Use
+ /// [`FFI_ArrowArrayStream::private_data`] and
[`FFI_ArrowArrayStream::set_private_data`].
Review Comment:
```suggestion
release: Option<unsafe extern "C" fn(arg1: *mut Self)>,
/// Private data used by the stream, owned by the release callback.
```
remove these details and instead just have them on the setters
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
