refactor: remove internal ffi module (#310)

molpopgen · web-flow · commit bd081db7760e · 2022-08-01T12:18:51.000-07:00
diff --git a/src/_macros.rs b/src/_macros.rs
@@ -127,51 +127,6 @@ macro_rules! unsafe_tsk_ragged_char_column_access {
     }};
 }
 
-macro_rules! drop_for_tskit_type {
-    ($name: ident, $drop: ident) => {
-        impl Drop for $name {
-            fn drop(&mut self) {
-                let rv = unsafe { $drop(&mut *self.inner) };
-                panic_on_tskit_error!(rv);
-            }
-        }
-    };
-}
-
-macro_rules! tskit_type_access {
-    ($name: ident, $ll_name: ty) => {
-        impl $crate::TskitTypeAccess<$ll_name> for $name {
-            fn as_ptr(&self) -> *const $ll_name {
-                &*self.inner
-            }
-
-            fn as_mut_ptr(&mut self) -> *mut $ll_name {
-                &mut *self.inner
-            }
-        }
-    };
-}
-
-macro_rules! build_tskit_type {
-    ($name: ident, $ll_name: ty, $drop: ident) => {
-        impl $crate::ffi::WrapTskitType<$ll_name> for $name {
-            fn wrap() -> Self {
-                use mbox::MBox;
-                let temp =
-                    unsafe { libc::malloc(std::mem::size_of::<$ll_name>()) as *mut $ll_name };
-                let nonnull = match std::ptr::NonNull::<$ll_name>::new(temp) {
-                    Some(x) => x,
-                    None => panic!("out of memory"),
-                };
-                let mbox = unsafe { MBox::from_non_null_raw(nonnull) };
-                $name { inner: mbox }
-            }
-        }
-        drop_for_tskit_type!($name, $drop);
-        tskit_type_access!($name, $ll_name);
-    };
-}
-
 macro_rules! metadata_to_vector {
     ($outer: ident, $table: expr, $row: expr) => {
         $crate::metadata::char_column_to_slice(
diff --git a/src/ffi.rs b/src/ffi.rs
diff --git a/src/lib.rs b/src/lib.rs
@@ -80,7 +80,6 @@ pub mod bindings;
 mod _macros; // Starts w/_ to be sorted at front by rustfmt!
 mod edge_table;
 pub mod error;
-mod ffi;
 mod flags;
 mod individual_table;
 pub mod metadata;
diff --git a/src/table_collection.rs b/src/table_collection.rs
@@ -1,6 +1,5 @@
 use crate::bindings as ll_bindings;
 use crate::error::TskitError;
-use crate::ffi::WrapTskitType;
 use crate::types::Bookmark;
 use crate::EdgeTable;
 use crate::IndividualTable;
@@ -60,14 +59,38 @@ use mbox::MBox;
 /// ```
 ///
 pub struct TableCollection {
-    pub(crate) inner: MBox<ll_bindings::tsk_table_collection_t>,
+    inner: MBox<ll_bindings::tsk_table_collection_t>,
 }
 
-build_tskit_type!(
-    TableCollection,
-    ll_bindings::tsk_table_collection_t,
-    tsk_table_collection_free
-);
+impl TskitTypeAccess<ll_bindings::tsk_table_collection_t> for TableCollection {
+    fn as_ptr(&self) -> *const ll_bindings::tsk_table_collection_t {
+        &*self.inner
+    }
+
+    fn as_mut_ptr(&mut self) -> *mut ll_bindings::tsk_table_collection_t {
+        &mut *self.inner
+    }
+}
+
+impl Drop for TableCollection {
+    fn drop(&mut self) {
+        let rv = unsafe { tsk_table_collection_free(self.as_mut_ptr()) };
+        assert_eq!(rv, 0);
+    }
+}
+
+/// Returns a pointer to an uninitialized tsk_table_collection_t
+pub(crate) fn uninit_table_collection() -> MBox<ll_bindings::tsk_table_collection_t> {
+    let temp = unsafe {
+        libc::malloc(std::mem::size_of::<ll_bindings::tsk_table_collection_t>())
+            as *mut ll_bindings::tsk_table_collection_t
+    };
+    let nonnull = match std::ptr::NonNull::<ll_bindings::tsk_table_collection_t>::new(temp) {
+        Some(x) => x,
+        None => panic!("out of memory"),
+    };
+    unsafe { MBox::from_non_null_raw(nonnull) }
+}
 
 impl TableCollection {
     /// Create a new table collection with a sequence length.
@@ -91,19 +114,27 @@ impl TableCollection {
                 expected: "sequence_length >= 0.0".to_string(),
             });
         }
-        let mut tables = Self::wrap();
-        let rv = unsafe { ll_bindings::tsk_table_collection_init(tables.as_mut_ptr(), 0) };
+        let mut mbox = uninit_table_collection();
+        let rv = unsafe { ll_bindings::tsk_table_collection_init(&mut *mbox, 0) };
         if rv < 0 {
             return Err(crate::error::TskitError::ErrorCode { code: rv });
         }
+        let mut tables = Self { inner: mbox };
         unsafe {
             (*tables.as_mut_ptr()).sequence_length = sequence_length.0;
         }
         Ok(tables)
     }
 
-    pub(crate) fn new_uninit() -> Self {
-        Self::wrap()
+    /// # Safety
+    ///
+    /// It is possible that the mbox's inner pointer has not be run through
+    /// tsk_table_collection_init, meaning that it is in an uninitialized state.
+    /// Or, it may be initialized and about to be used in a part of the C API
+    /// requiring an uninitialized table collection.
+    /// Consult the C API docs before using!
+    pub(crate) unsafe fn new_from_mbox(mbox: MBox<ll_bindings::tsk_table_collection_t>) -> Self {
+        Self { inner: mbox }
     }
 
     pub(crate) fn into_raw(self) -> Result<*mut ll_bindings::tsk_table_collection_t, TskitError> {
@@ -694,12 +725,13 @@ impl TableCollection {
     pub fn deepcopy(&self) -> Result<TableCollection, TskitError> {
         // The output is UNINITIALIZED tables,
         // else we leak memory
-        let mut copy = Self::new_uninit();
+        let mut inner = uninit_table_collection();
 
-        let rv =
-            unsafe { ll_bindings::tsk_table_collection_copy(self.as_ptr(), copy.as_mut_ptr(), 0) };
+        let rv = unsafe { ll_bindings::tsk_table_collection_copy(self.as_ptr(), &mut *inner, 0) };
 
-        handle_tsk_return_value!(rv, copy)
+        // SAFETY: we just initialized it.
+        // The C API doesn't free NULL pointers.
+        handle_tsk_return_value!(rv, unsafe { Self::new_from_mbox(inner) })
     }
 
     /// Return a [`crate::TreeSequence`] based on the tables.
diff --git a/src/trees.rs b/src/trees.rs
@@ -4,7 +4,6 @@ use std::ops::DerefMut;
 
 use crate::bindings as ll_bindings;
 use crate::error::TskitError;
-use crate::ffi::WrapTskitType;
 use crate::EdgeTable;
 use crate::IndividualTable;
 use crate::MigrationTable;
@@ -167,14 +166,6 @@ pub struct TreeSequence {
     pub(crate) inner: ll_bindings::tsk_treeseq_t,
 }
 
-impl crate::ffi::WrapTskitType<ll_bindings::tsk_treeseq_t> for TreeSequence {
-    fn wrap() -> Self {
-        let inner = std::mem::MaybeUninit::<ll_bindings::tsk_treeseq_t>::uninit();
-        let inner = unsafe { inner.assume_init() };
-        Self { inner }
-    }
-}
-
 unsafe impl Send for TreeSequence {}
 unsafe impl Sync for TreeSequence {}
 
@@ -243,17 +234,16 @@ impl TreeSequence {
         tables: TableCollection,
         flags: F,
     ) -> Result<Self, TskitError> {
-        let mut treeseq = Self::wrap();
+        let mut inner = std::mem::MaybeUninit::<ll_bindings::tsk_treeseq_t>::uninit();
         let mut flags: u32 = flags.into().bits();
         flags |= ll_bindings::TSK_TAKE_OWNERSHIP;
         let raw_tables_ptr = tables.into_raw()?;
         let rv =
-            unsafe { ll_bindings::tsk_treeseq_init(treeseq.as_mut_ptr(), raw_tables_ptr, flags) };
-        handle_tsk_return_value!(rv, treeseq)
-    }
-
-    fn new_uninit() -> Self {
-        Self::wrap()
+            unsafe { ll_bindings::tsk_treeseq_init(inner.as_mut_ptr(), raw_tables_ptr, flags) };
+        handle_tsk_return_value!(rv, {
+            let inner = unsafe { inner.assume_init() };
+            Self { inner }
+        })
     }
 
     /// Dump the tree sequence to file.
@@ -294,13 +284,15 @@ impl TreeSequence {
     ///
     /// [`TskitError`] will be raised if the underlying C library returns an error code.
     pub fn dump_tables(&self) -> Result<TableCollection, TskitError> {
-        let mut copy = TableCollection::new_uninit();
+        let mut inner = crate::table_collection::uninit_table_collection();
 
         let rv = unsafe {
-            ll_bindings::tsk_table_collection_copy((*self.as_ptr()).tables, copy.as_mut_ptr(), 0)
+            ll_bindings::tsk_table_collection_copy((*self.as_ptr()).tables, &mut *inner, 0)
         };
 
-        handle_tsk_return_value!(rv, copy)
+        // SAFETY: we just initialized it.
+        // The C API doesn't free NULL pointers.
+        handle_tsk_return_value!(rv, unsafe { TableCollection::new_from_mbox(inner) })
     }
 
     /// Create an iterator over trees.
@@ -428,7 +420,7 @@ impl TreeSequence {
     ) -> Result<(Self, Option<Vec<NodeId>>), TskitError> {
         // The output is an UNINITIALIZED treeseq,
         // else we leak memory.
-        let mut ts = Self::new_uninit();
+        let mut ts = MaybeUninit::<ll_bindings::tsk_treeseq_t>::uninit();
         let mut output_node_map: Vec<NodeId> = vec![];
         if idmap {
             output_node_map.resize(usize::try_from(self.nodes().num_rows())?, NodeId::NULL);
@@ -450,7 +442,10 @@ impl TreeSequence {
         handle_tsk_return_value!(
             rv,
             (
-                ts,
+                {
+                    let inner = unsafe { ts.assume_init() };
+                    Self { inner }
+                },
                 match idmap {
                     true => Some(output_node_map),
                     false => None,
