From 489630c200c31ff8ac845968a66ef2db86591b82 Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Sun, 4 Dec 2022 05:41:40 -0800
Subject: [PATCH] style: replace column access macro with generic fn

This change improves readability and declutters
some internal details.
---
 src/_macros.rs          | 41 -----------------------------------------
 src/edge_table.rs       | 27 +++++++++------------------
 src/individual_table.rs |  7 ++++++-
 src/lib.rs              |  1 +
 src/migration_table.rs  | 27 +++++++++++++--------------
 src/mutation_table.rs   | 14 ++++++--------
 src/node_table.rs       | 23 +++++++++++------------
 src/site_table.rs       |  8 +++-----
 src/sys.rs              | 32 ++++++++++++++++++++++++++++++++
 src/tree_interface.rs   | 15 ++++++++-------
 10 files changed, 89 insertions(+), 106 deletions(-)
 create mode 100644 src/sys.rs

diff --git a/src/_macros.rs b/src/_macros.rs
index 28727286f..5a6d19653 100644
--- a/src/_macros.rs
+++ b/src/_macros.rs
@@ -27,47 +27,6 @@ macro_rules! panic_on_tskit_error {
     };
 }
 
-macro_rules! unsafe_tsk_column_access {
-    ($i: expr, $lo: expr, $hi: expr, $owner: expr, $array: ident) => {{
-        let x = $crate::tsk_id_t::from($i);
-        if x < $lo || (x as $crate::tsk_size_t) >= $hi {
-            None
-        } else {
-            debug_assert!(!($owner).$array.is_null());
-            if !$owner.$array.is_null() {
-                // SAFETY: array is not null
-                // and we did our best effort
-                // on bounds checking
-                Some(unsafe { *$owner.$array.offset(x as isize) })
-            } else {
-                None
-            }
-        }
-    }};
-    ($i: expr, $lo: expr, $hi: expr, $owner: expr, $array: ident, $output_id_type: ty) => {{
-        let x = $crate::tsk_id_t::from($i);
-        if x < $lo || (x as $crate::tsk_size_t) >= $hi {
-            None
-        } else {
-            debug_assert!(!($owner).$array.is_null());
-            if !$owner.$array.is_null() {
-                // SAFETY: array is not null
-                // and we did our best effort
-                // on bounds checking
-                unsafe { Some(<$output_id_type>::from(*($owner.$array.offset(x as isize)))) }
-            } else {
-                None
-            }
-        }
-    }};
-}
-
-macro_rules! unsafe_tsk_column_access_and_map_into {
-    ($i: expr, $lo: expr, $hi: expr, $owner: expr, $array: ident) => {{
-        unsafe_tsk_column_access!($i, $lo, $hi, $owner, $array).map(|v| v.into())
-    }};
-}
-
 macro_rules! unsafe_tsk_ragged_column_access {
     ($i: expr, $lo: expr, $hi: expr, $owner: expr, $array: ident, $offset_array: ident, $offset_array_len: ident, $output_id_type: ty) => {{
         let i = $crate::SizeType::try_from($i).ok()?;
diff --git a/src/edge_table.rs b/src/edge_table.rs
index adc179cd6..aa1cf5221 100644
--- a/src/edge_table.rs
+++ b/src/edge_table.rs
@@ -2,6 +2,7 @@ use std::ptr::NonNull;
 
 use crate::bindings as ll_bindings;
 use crate::metadata;
+use crate::sys;
 use crate::Position;
 use crate::{tsk_id_t, TskitError};
 use crate::{EdgeId, NodeId};
@@ -180,14 +181,7 @@ impl EdgeTable {
     /// * `Some(parent)` if `u` is valid.
     /// * `None` otherwise.
     pub fn parent<E: Into<EdgeId> + Copy>(&self, row: E) -> Option<NodeId> {
-        unsafe_tsk_column_access!(
-            row.into(),
-            0,
-            self.num_rows(),
-            self.as_ref(),
-            parent,
-            NodeId
-        )
+        sys::tsk_column_access::<NodeId, _, _, _>(row.into(), self.as_ref().parent, self.num_rows())
     }
 
     /// Return the ``child`` value from row ``row`` of the table.
@@ -197,7 +191,7 @@ impl EdgeTable {
     /// * `Some(child)` if `u` is valid.
     /// * `None` otherwise.
     pub fn child<E: Into<EdgeId> + Copy>(&self, row: E) -> Option<NodeId> {
-        unsafe_tsk_column_access!(row.into(), 0, self.num_rows(), self.as_ref(), child, NodeId)
+        sys::tsk_column_access::<NodeId, _, _, _>(row.into(), self.as_ref().child, self.num_rows())
     }
 
     /// Return the ``left`` value from row ``row`` of the table.
@@ -207,14 +201,7 @@ impl EdgeTable {
     /// * `Some(position)` if `u` is valid.
     /// * `None` otherwise.
     pub fn left<E: Into<EdgeId> + Copy>(&self, row: E) -> Option<Position> {
-        unsafe_tsk_column_access!(
-            row.into(),
-            0,
-            self.num_rows(),
-            self.as_ref(),
-            left,
-            Position
-        )
+        sys::tsk_column_access::<Position, _, _, _>(row.into(), self.as_ref().left, self.num_rows())
     }
 
     /// Return the ``right`` value from row ``row`` of the table.
@@ -224,7 +211,11 @@ impl EdgeTable {
     /// * `Some(position)` if `u` is valid.
     /// * `None` otherwise.
     pub fn right<E: Into<EdgeId> + Copy>(&self, row: E) -> Option<Position> {
-        unsafe_tsk_column_access_and_map_into!(row.into(), 0, self.num_rows(), self.as_ref(), right)
+        sys::tsk_column_access::<Position, _, _, _>(
+            row.into(),
+            self.as_ref().right,
+            self.num_rows(),
+        )
     }
 
     /// Retrieve decoded metadata for a `row`.
diff --git a/src/individual_table.rs b/src/individual_table.rs
index b7fca8fa1..13e335653 100644
--- a/src/individual_table.rs
+++ b/src/individual_table.rs
@@ -2,6 +2,7 @@ use std::ptr::NonNull;
 
 use crate::bindings as ll_bindings;
 use crate::metadata;
+use crate::sys;
 use crate::IndividualFlags;
 use crate::IndividualId;
 use crate::Location;
@@ -171,7 +172,11 @@ impl IndividualTable {
     /// * `Some(flags)` if `row` is valid.
     /// * `None` otherwise.
     pub fn flags<I: Into<IndividualId> + Copy>(&self, row: I) -> Option<IndividualFlags> {
-        unsafe_tsk_column_access_and_map_into!(row.into(), 0, self.num_rows(), self.as_ref(), flags)
+        sys::tsk_column_access::<IndividualFlags, _, _, _>(
+            row.into(),
+            self.as_ref().flags,
+            self.num_rows(),
+        )
     }
 
     /// Return the locations for a given row.
diff --git a/src/lib.rs b/src/lib.rs
index 27c9b826c..81e5b1228 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -92,6 +92,7 @@ mod node_table;
 mod population_table;
 pub mod prelude;
 mod site_table;
+mod sys;
 mod table_collection;
 mod table_iterator;
 pub mod table_views;
diff --git a/src/migration_table.rs b/src/migration_table.rs
index 1bd8fe344..47308aa51 100644
--- a/src/migration_table.rs
+++ b/src/migration_table.rs
@@ -2,6 +2,7 @@ use std::ptr::NonNull;
 
 use crate::bindings as ll_bindings;
 use crate::metadata;
+use crate::sys;
 use crate::Position;
 use crate::SizeType;
 use crate::Time;
@@ -199,7 +200,7 @@ impl MigrationTable {
     /// * `Some(position)` if `row` is valid.
     /// * `None` otherwise.
     pub fn left<M: Into<MigrationId> + Copy>(&self, row: M) -> Option<Position> {
-        unsafe_tsk_column_access_and_map_into!(row.into(), 0, self.num_rows(), self.as_ref(), left)
+        sys::tsk_column_access::<Position, _, _, _>(row.into(), self.as_ref().left, self.num_rows())
     }
 
     /// Return the right coordinate for a given row.
@@ -209,7 +210,11 @@ impl MigrationTable {
     /// * `Some(positions)` if `row` is valid.
     /// * `None` otherwise.
     pub fn right<M: Into<MigrationId> + Copy>(&self, row: M) -> Option<Position> {
-        unsafe_tsk_column_access_and_map_into!(row.into(), 0, self.num_rows(), self.as_ref(), right)
+        sys::tsk_column_access::<Position, _, _, _>(
+            row.into(),
+            self.as_ref().right,
+            self.num_rows(),
+        )
     }
 
     /// Return the node for a given row.
@@ -219,7 +224,7 @@ impl MigrationTable {
     /// * `Some(node)` if `row` is valid.
     /// * `None` otherwise.
     pub fn node<M: Into<MigrationId> + Copy>(&self, row: M) -> Option<NodeId> {
-        unsafe_tsk_column_access!(row.into(), 0, self.num_rows(), self.as_ref(), node, NodeId)
+        sys::tsk_column_access::<NodeId, _, _, _>(row.into(), self.as_ref().node, self.num_rows())
     }
 
     /// Return the source population for a given row.
@@ -229,13 +234,10 @@ impl MigrationTable {
     /// * `Some(population)` if `row` is valid.
     /// * `None` otherwise.
     pub fn source<M: Into<MigrationId> + Copy>(&self, row: M) -> Option<PopulationId> {
-        unsafe_tsk_column_access!(
+        sys::tsk_column_access::<PopulationId, _, _, _>(
             row.into(),
-            0,
+            self.as_ref().source,
             self.num_rows(),
-            self.as_ref(),
-            source,
-            PopulationId
         )
     }
 
@@ -246,13 +248,10 @@ impl MigrationTable {
     /// * `Some(population)` if `row` is valid.
     /// * `None` otherwise.
     pub fn dest<M: Into<MigrationId> + Copy>(&self, row: M) -> Option<PopulationId> {
-        unsafe_tsk_column_access!(
+        sys::tsk_column_access::<PopulationId, _, _, _>(
             row.into(),
-            0,
+            self.as_ref().dest,
             self.num_rows(),
-            self.as_ref(),
-            dest,
-            PopulationId
         )
     }
 
@@ -263,7 +262,7 @@ impl MigrationTable {
     /// * `Some(time)` if `row` is valid.
     /// * `None` otherwise.
     pub fn time<M: Into<MigrationId> + Copy>(&self, row: M) -> Option<Time> {
-        unsafe_tsk_column_access_and_map_into!(row.into(), 0, self.num_rows(), self.as_ref(), time)
+        sys::tsk_column_access::<Time, _, _, _>(row.into(), self.as_ref().time, self.num_rows())
     }
 
     /// Retrieve decoded metadata for a `row`.
diff --git a/src/mutation_table.rs b/src/mutation_table.rs
index 56849d415..c479edb4a 100644
--- a/src/mutation_table.rs
+++ b/src/mutation_table.rs
@@ -2,6 +2,7 @@ use std::ptr::NonNull;
 
 use crate::bindings as ll_bindings;
 use crate::metadata;
+use crate::sys;
 use crate::SizeType;
 use crate::Time;
 use crate::{tsk_id_t, TskitError};
@@ -196,7 +197,7 @@ impl MutationTable {
     /// Will return [``IndexError``](crate::TskitError::IndexError)
     /// if ``row`` is out of range.
     pub fn site<M: Into<MutationId> + Copy>(&self, row: M) -> Option<SiteId> {
-        unsafe_tsk_column_access!(row.into(), 0, self.num_rows(), self.as_ref(), site, SiteId)
+        sys::tsk_column_access::<SiteId, _, _, _>(row.into(), self.as_ref().site, self.num_rows())
     }
 
     /// Return the ``node`` value from row ``row`` of the table.
@@ -206,7 +207,7 @@ impl MutationTable {
     /// Will return [``IndexError``](crate::TskitError::IndexError)
     /// if ``row`` is out of range.
     pub fn node<M: Into<MutationId> + Copy>(&self, row: M) -> Option<NodeId> {
-        unsafe_tsk_column_access!(row.into(), 0, self.num_rows(), self.as_ref(), node, NodeId)
+        sys::tsk_column_access::<NodeId, _, _, _>(row.into(), self.as_ref().node, self.num_rows())
     }
 
     /// Return the ``parent`` value from row ``row`` of the table.
@@ -216,13 +217,10 @@ impl MutationTable {
     /// Will return [``IndexError``](crate::TskitError::IndexError)
     /// if ``row`` is out of range.
     pub fn parent<M: Into<MutationId> + Copy>(&self, row: M) -> Option<MutationId> {
-        unsafe_tsk_column_access!(
+        sys::tsk_column_access::<MutationId, _, _, _>(
             row.into(),
-            0,
+            self.as_ref().parent,
             self.num_rows(),
-            self.as_ref(),
-            parent,
-            MutationId
         )
     }
 
@@ -233,7 +231,7 @@ impl MutationTable {
     /// Will return [``IndexError``](crate::TskitError::IndexError)
     /// if ``row`` is out of range.
     pub fn time<M: Into<MutationId> + Copy>(&self, row: M) -> Option<Time> {
-        unsafe_tsk_column_access!(row.into(), 0, self.num_rows(), self.as_ref(), time, Time)
+        sys::tsk_column_access::<Time, _, _, _>(row.into(), self.as_ref().time, self.num_rows())
     }
 
     /// Get the ``derived_state`` value from row ``row`` of the table.
diff --git a/src/node_table.rs b/src/node_table.rs
index 851d2cec6..cb8e8dc71 100644
--- a/src/node_table.rs
+++ b/src/node_table.rs
@@ -2,6 +2,7 @@ use std::ptr::NonNull;
 
 use crate::bindings as ll_bindings;
 use crate::metadata;
+use crate::sys;
 use crate::NodeFlags;
 use crate::SizeType;
 use crate::Time;
@@ -195,7 +196,7 @@ impl NodeTable {
     /// # }
     /// ```
     pub fn time<N: Into<NodeId> + Copy>(&self, row: N) -> Option<Time> {
-        unsafe_tsk_column_access!(row.into(), 0, self.num_rows(), self.as_ref(), time, Time)
+        sys::tsk_column_access::<Time, _, _, _>(row.into(), self.as_ref().time, self.num_rows())
     }
 
     /// Return the ``flags`` value from row ``row`` of the table.
@@ -220,7 +221,11 @@ impl NodeTable {
     /// # }
     /// ```
     pub fn flags<N: Into<NodeId> + Copy>(&self, row: N) -> Option<NodeFlags> {
-        unsafe_tsk_column_access_and_map_into!(row.into(), 0, self.num_rows(), self.as_ref(), flags)
+        sys::tsk_column_access::<NodeFlags, _, _, _>(
+            row.into(),
+            self.as_ref().flags,
+            self.num_rows(),
+        )
     }
 
     #[deprecated(since = "0.12.0", note = "use flags_slice_mut instead")]
@@ -265,13 +270,10 @@ impl NodeTable {
     /// * `Some(population)` if `row` is valid.
     /// * `None` otherwise.
     pub fn population<N: Into<NodeId> + Copy>(&self, row: N) -> Option<PopulationId> {
-        unsafe_tsk_column_access!(
+        sys::tsk_column_access::<PopulationId, _, _, _>(
             row.into(),
-            0,
+            self.as_ref().population,
             self.num_rows(),
-            self.as_ref(),
-            population,
-            PopulationId
         )
     }
 
@@ -311,13 +313,10 @@ impl NodeTable {
     /// * `Some(individual)` if `row` is valid.
     /// * `None` otherwise.
     pub fn individual<N: Into<NodeId> + Copy>(&self, row: N) -> Option<IndividualId> {
-        unsafe_tsk_column_access!(
+        sys::tsk_column_access::<IndividualId, _, _, _>(
             row.into(),
-            0,
+            self.as_ref().individual,
             self.num_rows(),
-            self.as_ref(),
-            individual,
-            IndividualId
         )
     }
 
diff --git a/src/site_table.rs b/src/site_table.rs
index 47d862709..7f6ef02fe 100644
--- a/src/site_table.rs
+++ b/src/site_table.rs
@@ -2,6 +2,7 @@ use std::ptr::NonNull;
 
 use crate::bindings as ll_bindings;
 use crate::metadata;
+use crate::sys;
 use crate::tsk_id_t;
 use crate::Position;
 use crate::SiteId;
@@ -166,13 +167,10 @@ impl SiteTable {
     /// * `Some(position)` if `row` is valid.
     /// * `None` otherwise.
     pub fn position<S: Into<SiteId> + Copy>(&self, row: S) -> Option<Position> {
-        unsafe_tsk_column_access!(
+        sys::tsk_column_access::<Position, _, _, _>(
             row.into(),
-            0,
+            self.as_ref().position,
             self.num_rows(),
-            self.as_ref(),
-            position,
-            Position
         )
     }
 
diff --git a/src/sys.rs b/src/sys.rs
new file mode 100644
index 000000000..78c85de29
--- /dev/null
+++ b/src/sys.rs
@@ -0,0 +1,32 @@
+use crate::bindings;
+
+fn tsk_column_access_detail<R: Into<bindings::tsk_id_t>, L: Into<bindings::tsk_size_t>, T: Copy>(
+    row: R,
+    column: *const T,
+    column_length: L,
+) -> Option<T> {
+    let row = row.into();
+    let column_length = column_length.into();
+    if row < 0 || (row as crate::tsk_size_t) >= column_length {
+        None
+    } else {
+        assert!(!column.is_null());
+        // SAFETY: pointer is not null.
+        // column_length is assumed to come directly
+        // from a table.
+        Some(unsafe { *column.offset(row as isize) })
+    }
+}
+
+pub fn tsk_column_access<
+    O: From<T>,
+    R: Into<bindings::tsk_id_t>,
+    L: Into<bindings::tsk_size_t>,
+    T: Copy,
+>(
+    row: R,
+    column: *const T,
+    column_length: L,
+) -> Option<O> {
+    tsk_column_access_detail(row, column, column_length).map(|v| v.into())
+}
diff --git a/src/tree_interface.rs b/src/tree_interface.rs
index 0dbf7b620..74ed70788 100644
--- a/src/tree_interface.rs
+++ b/src/tree_interface.rs
@@ -1,4 +1,5 @@
 use crate::bindings as ll_bindings;
+use crate::sys;
 use crate::tsk_id_t;
 use crate::tsk_size_t;
 use crate::NodeId;
@@ -297,7 +298,7 @@ impl TreeInterface {
     fn left_sample<N: Into<NodeId> + Copy>(&self, u: N) -> Option<NodeId> {
         // SAFETY: internal pointer cannot be NULL
         let ptr = unsafe { *self.as_ptr() };
-        unsafe_tsk_column_access!(u.into(), 0, self.num_nodes, ptr, left_sample, NodeId)
+        sys::tsk_column_access::<NodeId, _, _, _>(u.into(), ptr.left_sample, self.num_nodes)
     }
 
     // error if we are not tracking samples,
@@ -305,7 +306,7 @@ impl TreeInterface {
     fn right_sample<N: Into<NodeId> + Copy>(&self, u: N) -> Option<NodeId> {
         // SAFETY: internal pointer cannot be NULL
         let ptr = unsafe { *self.as_ptr() };
-        unsafe_tsk_column_access!(u.into(), 0, self.num_nodes, ptr, right_sample, NodeId)
+        sys::tsk_column_access::<NodeId, _, _, _>(u.into(), ptr.right_sample, self.num_nodes)
     }
 
     /// Return the `[left, right)` coordinates of the tree.
@@ -330,7 +331,7 @@ impl TreeInterface {
     pub fn parent<N: Into<NodeId> + Copy>(&self, u: N) -> Option<NodeId> {
         // SAFETY: internal pointer cannot be NULL
         let ptr = unsafe { *self.as_ptr() };
-        unsafe_tsk_column_access!(u.into(), 0, self.array_len, ptr, parent, NodeId)
+        sys::tsk_column_access::<NodeId, _, _, _>(u.into(), ptr.parent, self.array_len)
     }
 
     /// Get the left child of node `u`.
@@ -339,7 +340,7 @@ impl TreeInterface {
     pub fn left_child<N: Into<NodeId> + Copy>(&self, u: N) -> Option<NodeId> {
         // SAFETY: internal pointer cannot be NULL
         let ptr = unsafe { *self.as_ptr() };
-        unsafe_tsk_column_access!(u.into(), 0, self.array_len, ptr, left_child, NodeId)
+        sys::tsk_column_access::<NodeId, _, _, _>(u.into(), ptr.left_child, self.array_len)
     }
 
     /// Get the right child of node `u`.
@@ -348,7 +349,7 @@ impl TreeInterface {
     pub fn right_child<N: Into<NodeId> + Copy>(&self, u: N) -> Option<NodeId> {
         // SAFETY: internal pointer cannot be NULL
         let ptr = unsafe { *self.as_ptr() };
-        unsafe_tsk_column_access!(u.into(), 0, self.array_len, ptr, right_child, NodeId)
+        sys::tsk_column_access::<NodeId, _, _, _>(u.into(), ptr.right_child, self.array_len)
     }
 
     /// Get the left sib of node `u`.
@@ -357,7 +358,7 @@ impl TreeInterface {
     pub fn left_sib<N: Into<NodeId> + Copy>(&self, u: N) -> Option<NodeId> {
         // SAFETY: internal pointer cannot be NULL
         let ptr = unsafe { *self.as_ptr() };
-        unsafe_tsk_column_access!(u.into(), 0, self.array_len, ptr, left_sib, NodeId)
+        sys::tsk_column_access::<NodeId, _, _, _>(u.into(), ptr.left_sib, self.array_len)
     }
 
     /// Get the right sib of node `u`.
@@ -366,7 +367,7 @@ impl TreeInterface {
     pub fn right_sib<N: Into<NodeId> + Copy>(&self, u: N) -> Option<NodeId> {
         // SAFETY: internal pointer cannot be NULL
         let ptr = unsafe { *self.as_ptr() };
-        unsafe_tsk_column_access!(u.into(), 0, self.array_len, ptr, right_sib, NodeId)
+        sys::tsk_column_access::<NodeId, _, _, _>(u.into(), ptr.right_sib, self.array_len)
     }
 
     /// Obtain the list of samples for the current tree/tree sequence