From 1ec7013d7b9a4ecc4a2b80e5ecf155c77df823c6 Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Tue, 6 Dec 2022 10:39:45 -0800
Subject: [PATCH] refactor: move Drop details for TreeSequence to sys.

* works via LLTreeSeq.free()
* add tests of sys::Error
---
 src/error.rs |  6 +----
 src/sys.rs   | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 src/trees.rs |  7 ------
 3 files changed, 62 insertions(+), 14 deletions(-)

diff --git a/src/error.rs b/src/error.rs
index 91e6d9942..8b23a4432 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -100,11 +100,7 @@ pub fn panic_on_tskit_error(code: i32) {
 /// This function must allocate a C string, which may panic
 /// if the system runs out of memory.
 pub fn get_tskit_error_message(code: i32) -> String {
-    let c_str = unsafe { std::ffi::CStr::from_ptr(crate::bindings::tsk_strerror(code)) };
-    c_str
-        .to_str()
-        .expect("failed to convert c_str to &str")
-        .to_owned()
+    sys::get_tskit_error_message(code)
 }
 
 /// Given an instance of [``TskReturnValue``](crate::TskReturnValue),
diff --git a/src/sys.rs b/src/sys.rs
index 711f44e65..60114db8f 100644
--- a/src/sys.rs
+++ b/src/sys.rs
@@ -15,9 +15,9 @@ use bindings::tsk_site_table_t;
 #[non_exhaustive]
 #[derive(Error, Debug)]
 pub enum Error {
-    #[error("{}", 0)]
+    #[error("{}", *.0)]
     Message(String),
-    #[error("{}", 0)]
+    #[error("{}", get_tskit_error_message(*.0))]
     Code(i32),
 }
 
@@ -150,6 +150,22 @@ impl LLTreeSeq {
     pub fn num_samples(&self) -> bindings::tsk_size_t {
         unsafe { bindings::tsk_treeseq_get_num_samples(self.as_ptr()) }
     }
+
+    fn free(&mut self) -> Result<(), Error> {
+        match unsafe { bindings::tsk_treeseq_free(self.as_mut_ptr()) } {
+            code if code < 0 => Err(Error::Code(code)),
+            _ => Ok(()),
+        }
+    }
+}
+
+impl Drop for LLTreeSeq {
+    fn drop(&mut self) {
+        match self.free() {
+            Ok(_) => (),
+            Err(e) => panic!("{:?}", e),
+        }
+    }
 }
 
 fn tsk_column_access_detail<R: Into<bindings::tsk_id_t>, L: Into<bindings::tsk_size_t>, T: Copy>(
@@ -256,3 +272,46 @@ pub fn generate_slice_mut<'a, L: Into<bindings::tsk_size_t>, I, O>(
     // SAFETY: pointer is not null, length comes from C API
     unsafe { std::slice::from_raw_parts_mut(data.cast::<O>(), length.into() as usize) }
 }
+
+pub fn get_tskit_error_message(code: i32) -> String {
+    let c_str = unsafe { std::ffi::CStr::from_ptr(crate::bindings::tsk_strerror(code)) };
+    c_str
+        .to_str()
+        .expect("failed to convert c_str to &str")
+        .to_owned()
+}
+
+#[test]
+fn test_error_message() {
+    fn foo() -> Result<(), Error> {
+        Err(Error::Message("foobar".to_owned()))
+    }
+
+    let msg = "foobar".to_owned();
+    match foo() {
+        Err(Error::Message(m)) => assert_eq!(m, msg),
+        _ => panic!("unexpected match"),
+    }
+}
+
+#[test]
+fn test_error_code() {
+    fn foo() -> Result<(), Error> {
+        Err(Error::Code(-202))
+    }
+
+    match foo() {
+        Err(Error::Code(x)) => {
+            assert_eq!(x, -202);
+        }
+        _ => panic!("unexpected match"),
+    }
+
+    match foo() {
+        Err(e) => {
+            let m = format!("{}", e);
+            assert_eq!(&m, "Node out of bounds. (TSK_ERR_NODE_OUT_OF_BOUNDS)");
+        }
+        _ => panic!("unexpected match"),
+    }
+}
diff --git a/src/trees.rs b/src/trees.rs
index 8ab72072f..3c908d8bd 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -191,13 +191,6 @@ pub struct TreeSequence {
 unsafe impl Send for TreeSequence {}
 unsafe impl Sync for TreeSequence {}
 
-impl Drop for TreeSequence {
-    fn drop(&mut self) {
-        let rv = unsafe { ll_bindings::tsk_treeseq_free(self.as_mut_ptr()) };
-        assert_eq!(rv, 0);
-    }
-}
-
 impl TreeSequence {
     /// Create a tree sequence from a [`TableCollection`].
     /// In general, [`TableCollection::tree_sequence`] may be preferred.