From a7efd22d89c9468e09bb87750ab1590fc5a837f5 Mon Sep 17 00:00:00 2001 From: "Kevin R. Thornton" Date: Sun, 4 Dec 2022 15:39:58 -0800 Subject: [PATCH] refactor: add low-level table wrappers to sys * transparent newtypes over NonNull * moves lots of unsafe from table modules to sys. --- src/edge_table.rs | 13 ++++------- src/individual_table.rs | 13 ++++------- src/migration_table.rs | 13 ++++------- src/mutation_table.rs | 13 ++++------- src/node_table.rs | 13 ++++------- src/population_table.rs | 14 +++++------- src/provenance.rs | 14 ++++-------- src/site_table.rs | 13 ++++------- src/sys.rs | 49 ++++++++++++++++++++++++++++++++++++++++- src/table_collection.rs | 2 +- 10 files changed, 82 insertions(+), 75 deletions(-) diff --git a/src/edge_table.rs b/src/edge_table.rs index 9407f8f41..c9f16b3f6 100644 --- a/src/edge_table.rs +++ b/src/edge_table.rs @@ -1,5 +1,3 @@ -use std::ptr::NonNull; - use crate::bindings as ll_bindings; use crate::metadata; use crate::sys; @@ -148,22 +146,19 @@ impl<'a> streaming_iterator::StreamingIterator for EdgeTableRowView<'a> { #[repr(transparent)] #[derive(Debug)] pub struct EdgeTable { - pub(crate) table_: NonNull, + pub(crate) table_: sys::LLEdgeTableRef, } impl EdgeTable { pub(crate) fn new_from_table( edges: *mut ll_bindings::tsk_edge_table_t, ) -> Result { - let n = NonNull::new(edges).ok_or_else(|| { - TskitError::LibraryError("null pointer to tsk_edge_table_t".to_string()) - })?; - Ok(EdgeTable { table_: n }) + let table_ = sys::LLEdgeTableRef::new_from_table(edges)?; + Ok(EdgeTable { table_ }) } pub(crate) fn as_ref(&self) -> &ll_bindings::tsk_edge_table_t { - // SAFETY: NonNull - unsafe { self.table_.as_ref() } + self.table_.as_ref() } /// Return the number of rows diff --git a/src/individual_table.rs b/src/individual_table.rs index 28fa20b57..f517440fc 100644 --- a/src/individual_table.rs +++ b/src/individual_table.rs @@ -1,5 +1,3 @@ -use std::ptr::NonNull; - use crate::bindings as ll_bindings; use crate::metadata; use crate::sys; @@ -105,7 +103,7 @@ impl<'a> streaming_iterator::StreamingIterator for IndividualTableRowView<'a> { /// [`crate::table_views::TableViews`] #[derive(Debug)] pub struct IndividualTable { - table_: NonNull, + table_: sys::LLIndividualTableRef, } fn make_individual_table_row(table: &IndividualTable, pos: tsk_id_t) -> Option { @@ -146,15 +144,12 @@ impl IndividualTable { pub(crate) fn new_from_table( individuals: *mut ll_bindings::tsk_individual_table_t, ) -> Result { - let n = NonNull::new(individuals).ok_or_else(|| { - TskitError::LibraryError("null pointer to tsk_individual_table_t".to_string()) - })?; - Ok(IndividualTable { table_: n }) + let table_ = sys::LLIndividualTableRef::new_from_table(individuals)?; + Ok(IndividualTable { table_ }) } pub(crate) fn as_ref(&self) -> &ll_bindings::tsk_individual_table_t { - // SAFETY: NonNull - unsafe { self.table_.as_ref() } + self.table_.as_ref() } raw_metadata_getter_for_tables!(IndividualId); diff --git a/src/migration_table.rs b/src/migration_table.rs index 688501708..b8a10f90d 100644 --- a/src/migration_table.rs +++ b/src/migration_table.rs @@ -1,5 +1,3 @@ -use std::ptr::NonNull; - use crate::bindings as ll_bindings; use crate::metadata; use crate::sys; @@ -167,22 +165,19 @@ impl<'a> streaming_iterator::StreamingIterator for MigrationTableRowView<'a> { /// [`crate::table_views::TableViews`] #[derive(Debug)] pub struct MigrationTable { - table_: NonNull, + table_: sys::LLMigrationTableRef, } impl MigrationTable { pub(crate) fn new_from_table( migrations: *mut ll_bindings::tsk_migration_table_t, ) -> Result { - let n = NonNull::new(migrations).ok_or_else(|| { - TskitError::LibraryError("null pointer to tsk_migration_table_t".to_string()) - })?; - Ok(MigrationTable { table_: n }) + let table_ = sys::LLMigrationTableRef::new_from_table(migrations)?; + Ok(MigrationTable { table_ }) } pub(crate) fn as_ref(&self) -> &ll_bindings::tsk_migration_table_t { - // SAFETY: NonNull - unsafe { self.table_.as_ref() } + self.table_.as_ref() } /// Return the number of rows diff --git a/src/mutation_table.rs b/src/mutation_table.rs index 17f632526..825a912ae 100644 --- a/src/mutation_table.rs +++ b/src/mutation_table.rs @@ -1,5 +1,3 @@ -use std::ptr::NonNull; - use crate::bindings as ll_bindings; use crate::metadata; use crate::sys; @@ -164,22 +162,19 @@ impl<'a> streaming_iterator::StreamingIterator for MutationTableRowView<'a> { /// [`crate::table_views::TableViews`] #[derive(Debug)] pub struct MutationTable { - table_: NonNull, + table_: sys::LLMutationTableRef, } impl MutationTable { pub(crate) fn new_from_table( mutations: *mut ll_bindings::tsk_mutation_table_t, ) -> Result { - let n = NonNull::new(mutations).ok_or_else(|| { - TskitError::LibraryError("null pointer to tsk_mutation_table_t".to_string()) - })?; - Ok(MutationTable { table_: n }) + let table_ = sys::LLMutationTableRef::new_from_table(mutations)?; + Ok(MutationTable { table_ }) } pub(crate) fn as_ref(&self) -> &ll_bindings::tsk_mutation_table_t { - // SAFETY: NonNull - unsafe { self.table_.as_ref() } + self.table_.as_ref() } /// Return the number of rows. diff --git a/src/node_table.rs b/src/node_table.rs index b80ba2fa4..e2c6d5b68 100644 --- a/src/node_table.rs +++ b/src/node_table.rs @@ -1,5 +1,3 @@ -use std::ptr::NonNull; - use crate::bindings as ll_bindings; use crate::metadata; use crate::sys; @@ -148,22 +146,19 @@ impl<'a> streaming_iterator::StreamingIterator for NodeTableRowView<'a> { /// [`crate::table_views::TableViews`] #[derive(Debug)] pub struct NodeTable { - table_: NonNull, + table_: sys::LLNodeTableRef, } impl NodeTable { pub(crate) fn new_from_table( nodes: *mut ll_bindings::tsk_node_table_t, ) -> Result { - let n = NonNull::new(nodes).ok_or_else(|| { - TskitError::LibraryError("null pointer to tsk_node_table_t".to_string()) - })?; - Ok(NodeTable { table_: n }) + let table_ = sys::LLNodeTableRef::new_from_table(nodes)?; + Ok(NodeTable { table_ }) } pub(crate) fn as_ref(&self) -> &ll_bindings::tsk_node_table_t { - // SAFETY: NonNull - unsafe { self.table_.as_ref() } + self.table_.as_ref() } /// Return the number of rows diff --git a/src/population_table.rs b/src/population_table.rs index 7ca10cec6..ae955c033 100644 --- a/src/population_table.rs +++ b/src/population_table.rs @@ -1,7 +1,6 @@ -use std::ptr::NonNull; - use crate::bindings as ll_bindings; use crate::metadata; +use crate::sys; use crate::tsk_id_t; use crate::PopulationId; use crate::SizeType; @@ -116,22 +115,19 @@ impl<'a> streaming_iterator::StreamingIterator for PopulationTableRowView<'a> { #[repr(transparent)] #[derive(Debug)] pub struct PopulationTable { - table_: NonNull, + table_: sys::LLPopulationTableRef, } impl PopulationTable { pub(crate) fn new_from_table( populations: *mut ll_bindings::tsk_population_table_t, ) -> Result { - let n = NonNull::new(populations).ok_or_else(|| { - TskitError::LibraryError("null pointer to tsk_population_table_t".to_string()) - })?; - Ok(PopulationTable { table_: n }) + let table_ = sys::LLPopulationTableRef::new_from_table(populations)?; + Ok(PopulationTable { table_ }) } pub(crate) fn as_ref(&self) -> &ll_bindings::tsk_population_table_t { - // SAFETY: NonNull - unsafe { self.table_.as_ref() } + self.table_.as_ref() } raw_metadata_getter_for_tables!(PopulationId); diff --git a/src/provenance.rs b/src/provenance.rs index df289b4d0..c832a0f4e 100644 --- a/src/provenance.rs +++ b/src/provenance.rs @@ -10,8 +10,6 @@ //! [`ProvenanceTable::iter`]. //! -use std::ptr::NonNull; - use crate::bindings as ll_bindings; use crate::sys; use crate::SizeType; @@ -141,23 +139,19 @@ impl<'a> streaming_iterator::StreamingIterator for ProvenanceTableRowView<'a> { /// #[derive(Debug)] pub struct ProvenanceTable { - table_: NonNull, + table_: sys::LLProvenanceTableRef, } impl ProvenanceTable { pub(crate) fn new_from_table( provenances: *mut ll_bindings::tsk_provenance_table_t, ) -> Result { - // FIXME: unwrap - let n = NonNull::new(provenances).ok_or_else(|| { - crate::TskitError::LibraryError("null pointer to tsk_provenance_table_t".to_string()) - })?; - Ok(ProvenanceTable { table_: n }) + let table_ = sys::LLProvenanceTableRef::new_from_table(provenances)?; + Ok(ProvenanceTable { table_ }) } pub(crate) fn as_ref(&self) -> &ll_bindings::tsk_provenance_table_t { - // SAFETY: NonNull - unsafe { self.table_.as_ref() } + self.table_.as_ref() } /// Return the number of rows diff --git a/src/site_table.rs b/src/site_table.rs index b23bce145..54b6b9beb 100644 --- a/src/site_table.rs +++ b/src/site_table.rs @@ -1,5 +1,3 @@ -use std::ptr::NonNull; - use crate::bindings as ll_bindings; use crate::metadata; use crate::sys; @@ -134,22 +132,19 @@ impl<'a> streaming_iterator::StreamingIterator for SiteTableRowView<'a> { /// [`crate::table_views::TableViews`] #[derive(Debug)] pub struct SiteTable { - table_: NonNull, + table_: sys::LLSiteTableRef, } impl SiteTable { pub(crate) fn new_from_table( sites: *mut ll_bindings::tsk_site_table_t, ) -> Result { - let n = NonNull::new(sites).ok_or_else(|| { - TskitError::LibraryError("null pointer to tsk_site_table_t".to_string()) - })?; - Ok(SiteTable { table_: n }) + let table_ = sys::LLSiteTableRef::new_from_table(sites)?; + Ok(SiteTable { table_ }) } pub(crate) fn as_ref(&self) -> &ll_bindings::tsk_site_table_t { - // SAFETY: NonNull - unsafe { self.table_.as_ref() } + self.table_.as_ref() } raw_metadata_getter_for_tables!(SiteId); diff --git a/src/sys.rs b/src/sys.rs index f91c912fe..05523ad8d 100644 --- a/src/sys.rs +++ b/src/sys.rs @@ -1,4 +1,51 @@ -use crate::bindings; +use crate::{bindings, TskitError}; +use bindings::tsk_edge_table_t; +use bindings::tsk_individual_table_t; +use bindings::tsk_migration_table_t; +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::ptr::NonNull; + +#[cfg(feature = "provenance")] +use bindings::tsk_provenance_table_t; + +macro_rules! basic_lltableref_impl { + ($lltable: ident, $tsktable: ident) => { + #[repr(transparent)] + #[derive(Debug)] + pub struct $lltable(NonNull); + + impl $lltable { + 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) + })?; + Ok(Self(internal)) + } + + pub fn as_ref(&self) -> &$tsktable { + // SAFETY: we cannot get this far w/o + // going through new_from_table and that + // fn protects us from null ptrs + unsafe { self.0.as_ref() } + } + } + }; +} + +basic_lltableref_impl!(LLEdgeTableRef, tsk_edge_table_t); +basic_lltableref_impl!(LLNodeTableRef, tsk_node_table_t); +basic_lltableref_impl!(LLMutationTableRef, tsk_mutation_table_t); +basic_lltableref_impl!(LLSiteTableRef, tsk_site_table_t); +basic_lltableref_impl!(LLMigrationTableRef, tsk_migration_table_t); +basic_lltableref_impl!(LLPopulationTableRef, tsk_population_table_t); +basic_lltableref_impl!(LLIndividualTableRef, tsk_individual_table_t); + +#[cfg(feature = "provenance")] +basic_lltableref_impl!(LLProvenanceTableRef, tsk_provenance_table_t); fn tsk_column_access_detail, L: Into, T: Copy>( row: R, diff --git a/src/table_collection.rs b/src/table_collection.rs index 244951693..0f3e6c817 100644 --- a/src/table_collection.rs +++ b/src/table_collection.rs @@ -109,7 +109,7 @@ impl TableCollection { // AHA? assert!(std::ptr::eq( &mbox.as_ref().edges as *const ll_bindings::tsk_edge_table_t, - views.edges().table_.as_ptr() as *const ll_bindings::tsk_edge_table_t + views.edges().table_.as_ref() as *const ll_bindings::tsk_edge_table_t )); let mut tables = Self { inner: mbox,