From 94a033c86056b6b8238c61479637602bb7fa7b87 Mon Sep 17 00:00:00 2001 From: "Kevin R. Thornton" Date: Tue, 6 Dec 2022 10:08:30 -0800 Subject: [PATCH] refactor: sys module no longer depends on TskitError * sys now defines its own Error. * impl From for TskitError --- src/error.rs | 10 +++++++++ src/sys.rs | 58 +++++++++++++++++++++++++++++++++------------------- src/trees.rs | 8 ++++++-- 3 files changed, 53 insertions(+), 23 deletions(-) diff --git a/src/error.rs b/src/error.rs index 1da4f610c..91e6d9942 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,5 +1,6 @@ //! Error handling +use crate::sys; use crate::TskReturnValue; use thiserror::Error; @@ -36,6 +37,15 @@ pub enum TskitError { LibraryError(String), } +impl From for TskitError { + fn from(error: sys::Error) -> Self { + match error { + sys::Error::Message(msg) => TskitError::LibraryError(msg), + sys::Error::Code(code) => TskitError::ErrorCode { code }, + } + } +} + /// Takes the return code from a tskit /// function and panics if the code indicates /// an error. The error message is included diff --git a/src/sys.rs b/src/sys.rs index 93e44dcb0..711f44e65 100644 --- a/src/sys.rs +++ b/src/sys.rs @@ -1,4 +1,9 @@ -use crate::{bindings, TskitError}; +use std::ffi::CString; +use std::ptr::NonNull; + +use thiserror::Error; + +use crate::bindings; use bindings::tsk_edge_table_t; use bindings::tsk_individual_table_t; use bindings::tsk_migration_table_t; @@ -6,8 +11,15 @@ use bindings::tsk_mutation_table_t; use bindings::tsk_node_table_t; use bindings::tsk_population_table_t; use bindings::tsk_site_table_t; -use std::ffi::CString; -use std::ptr::NonNull; + +#[non_exhaustive] +#[derive(Error, Debug)] +pub enum Error { + #[error("{}", 0)] + Message(String), + #[error("{}", 0)] + Code(i32), +} #[cfg(feature = "provenance")] use bindings::tsk_provenance_table_t; @@ -19,10 +31,10 @@ macro_rules! basic_lltableref_impl { pub struct $lltable(NonNull); impl $lltable { - pub fn new_from_table(table: *mut $tsktable) -> Result { + pub fn new_from_table(table: *mut $tsktable) -> Result { let internal = NonNull::new(table).ok_or_else(|| { let msg = format!("null pointer to {}", stringify!($tsktable)); - TskitError::LibraryError(msg) + Error::Message(msg) })?; Ok(Self(internal)) } @@ -55,12 +67,14 @@ impl LLTreeSeq { pub fn new( tables: *mut bindings::tsk_table_collection_t, flags: bindings::tsk_flags_t, - ) -> Result { + ) -> Result { let mut inner = std::mem::MaybeUninit::::uninit(); let mut flags = flags; flags |= bindings::TSK_TAKE_OWNERSHIP; - let rv = unsafe { bindings::tsk_treeseq_init(inner.as_mut_ptr(), tables, flags) }; - handle_tsk_return_value!(rv, Self(unsafe { inner.assume_init() })) + match unsafe { bindings::tsk_treeseq_init(inner.as_mut_ptr(), tables, flags) } { + code if code < 0 => Err(Error::Code(code)), + _ => Ok(Self(unsafe { inner.assume_init() })), + } } pub fn as_ref(&self) -> &bindings::tsk_treeseq_t { @@ -80,7 +94,7 @@ impl LLTreeSeq { samples: &[bindings::tsk_id_t], options: bindings::tsk_flags_t, idmap: *mut bindings::tsk_id_t, - ) -> Result { + ) -> Result { // The output is an UNINITIALIZED treeseq, // else we leak memory. let mut ts = std::mem::MaybeUninit::::uninit(); @@ -102,18 +116,18 @@ impl LLTreeSeq { // and tsk_treeseq_free uses safe methods // to clean up. unsafe { bindings::tsk_treeseq_free(ts.as_mut_ptr()) }; + Err(Error::Code(rv)) + } else { + Ok(Self(init)) } - handle_tsk_return_value!(rv, Self(init)) } - pub fn dump( - &self, - filename: CString, - options: bindings::tsk_flags_t, - ) -> Result { + pub fn dump(&self, filename: CString, options: bindings::tsk_flags_t) -> Result { // SAFETY: self pointer is not null - let rv = unsafe { bindings::tsk_treeseq_dump(self.as_ptr(), filename.as_ptr(), options) }; - handle_tsk_return_value!(rv) + match unsafe { bindings::tsk_treeseq_dump(self.as_ptr(), filename.as_ptr(), options) } { + code if code < 0 => Err(Error::Code(code)), + code => Ok(code), + } } pub fn num_trees(&self) -> bindings::tsk_size_t { @@ -121,14 +135,16 @@ impl LLTreeSeq { unsafe { bindings::tsk_treeseq_get_num_trees(self.as_ptr()) } } - pub fn kc_distance(&self, other: &Self, lambda: f64) -> Result { + pub fn kc_distance(&self, other: &Self, lambda: f64) -> Result { let mut kc: f64 = f64::NAN; let kcp: *mut f64 = &mut kc; // SAFETY: self pointer is not null - let code = unsafe { + match unsafe { bindings::tsk_treeseq_kc_distance(self.as_ptr(), other.as_ptr(), lambda, kcp) - }; - handle_tsk_return_value!(code, kc) + } { + code if code < 0 => Err(Error::Code(code)), + _ => Ok(kc), + } } pub fn num_samples(&self) -> bindings::tsk_size_t { diff --git a/src/trees.rs b/src/trees.rs index a91a70f3f..8ab72072f 100644 --- a/src/trees.rs +++ b/src/trees.rs @@ -282,7 +282,9 @@ impl TreeSequence { let c_str = std::ffi::CString::new(filename).map_err(|_| { TskitError::LibraryError("call to ffi::Cstring::new failed".to_string()) })?; - self.inner.dump(c_str, options.into().bits()) + self.inner + .dump(c_str, options.into().bits()) + .map_err(|e| e.into()) } /// Load from a file. @@ -404,7 +406,9 @@ impl TreeSequence { /// * `lambda` specifies the relative weight of topology and branch length. /// See [`TreeInterface::kc_distance`] for more details. pub fn kc_distance(&self, other: &TreeSequence, lambda: f64) -> Result { - self.inner.kc_distance(&other.inner, lambda) + self.inner + .kc_distance(&other.inner, lambda) + .map_err(|e| e.into()) } // FIXME: document