Skip to content

Commit b09816b

Browse files
authored
refactor: replace Deref with delegation of TableViews API (#409)
The previous use of Deref/DerefMut was an anti-pattern. We now use the delegate crate to provide the same API. BREAKING CHANGE: code generic over the Deref behavior is now broken.
1 parent 63f9251 commit b09816b

File tree

5 files changed

+150
-43
lines changed

5 files changed

+150
-43
lines changed

Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ serde_json = {version = "1.0.67", optional = true}
2727
bincode = {version = "1.3.1", optional = true}
2828
tskit-derive = {version = "0.2.0", path = "tskit-derive", optional = true}
2929
mbox = "0.6.0"
30+
delegate = "0.8.0"
3031

3132
[dev-dependencies]
3233
anyhow = {version = "1.0.66"}

src/_macros.rs

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1191,6 +1191,121 @@ macro_rules! build_table_column_slice_mut_getter {
11911191
};
11921192
}
11931193

1194+
macro_rules! delegate_table_view_api {
1195+
() => {
1196+
delegate::delegate! {
1197+
to self.views {
1198+
/// Get reference to the [``EdgeTable``](crate::EdgeTable).
1199+
pub fn edges(&self) -> &crate::EdgeTable;
1200+
/// Get reference to the [``IndividualTable``](crate::IndividualTable).
1201+
pub fn individuals(&self) -> &crate::IndividualTable;
1202+
/// Get reference to the [``MigrationTable``](crate::MigrationTable).
1203+
pub fn migrations(&self) -> &crate::MigrationTable;
1204+
/// Get reference to the [``MutationTable``](crate::MutationTable).
1205+
pub fn mutations(&self) -> &crate::MutationTable;
1206+
/// Get reference to the [``NodeTable``](crate::NodeTable).
1207+
pub fn nodes(&self) -> &crate::NodeTable;
1208+
/// Get reference to the [``PopulationTable``](crate::PopulationTable).
1209+
pub fn populations(&self) -> &crate::PopulationTable;
1210+
/// Get reference to the [``SiteTable``](crate::SiteTable).
1211+
pub fn sites(&self) -> &crate::SiteTable;
1212+
1213+
#[cfg(feature = "provenance")]
1214+
#[cfg_attr(doc_cfg, doc(cfg(feature = "provenance")))]
1215+
/// Get reference to the [``ProvenanceTable``](crate::provenance::ProvenanceTable)
1216+
pub fn provenances(&self) -> &crate::provenance::ProvenanceTable ;
1217+
1218+
/// Return an iterator over the individuals.
1219+
pub fn individuals_iter(&self) -> impl Iterator<Item = crate::IndividualTableRow> + '_;
1220+
/// Return an iterator over the nodes.
1221+
pub fn nodes_iter(&self) -> impl Iterator<Item = crate::NodeTableRow> + '_;
1222+
/// Return an iterator over the edges.
1223+
pub fn edges_iter(&self) -> impl Iterator<Item = crate::EdgeTableRow> + '_;
1224+
/// Return an iterator over the migrations.
1225+
pub fn migrations_iter(&self) -> impl Iterator<Item = crate::MigrationTableRow> + '_;
1226+
/// Return an iterator over the mutations.
1227+
pub fn mutations_iter(&self) -> impl Iterator<Item = crate::MutationTableRow> + '_;
1228+
/// Return an iterator over the populations.
1229+
pub fn populations_iter(&self) -> impl Iterator<Item = crate::PopulationTableRow> + '_;
1230+
/// Return an iterator over the sites.
1231+
pub fn sites_iter(&self) -> impl Iterator<Item = crate::SiteTableRow> + '_;
1232+
1233+
#[cfg(feature = "provenance")]
1234+
#[cfg_attr(doc_cfg, doc(cfg(feature = "provenance")))]
1235+
/// Return an iterator over provenances
1236+
pub fn provenances_iter(&self,) -> impl Iterator<Item = crate::provenance::ProvenanceTableRow> + '_;
1237+
1238+
/// Obtain a vector containing the indexes ("ids")
1239+
/// of all nodes for which [`crate::TSK_NODE_IS_SAMPLE`]
1240+
/// is `true`.
1241+
///
1242+
/// The provided implementation dispatches to
1243+
/// [`crate::NodeTable::samples_as_vector`].
1244+
pub fn samples_as_vector(&self) -> Vec<crate::NodeId>;
1245+
1246+
/// Obtain a vector containing the indexes ("ids") of all nodes
1247+
/// satisfying a certain criterion.
1248+
///
1249+
/// The provided implementation dispatches to
1250+
/// [`crate::NodeTable::create_node_id_vector`].
1251+
///
1252+
/// # Parameters
1253+
///
1254+
/// * `f`: a function. The function is passed the current table
1255+
/// collection and each [`crate::node_table::NodeTableRow`].
1256+
/// If `f` returns `true`, the index of that row is included
1257+
/// in the return value.
1258+
///
1259+
/// # Examples
1260+
///
1261+
/// Get all nodes with time > 0.0:
1262+
///
1263+
/// ```
1264+
/// use tskit::bindings::tsk_id_t;
1265+
///
1266+
/// let mut tables = tskit::TableCollection::new(100.).unwrap();
1267+
/// tables
1268+
/// .add_node(tskit::TSK_NODE_IS_SAMPLE, 0.0, tskit::PopulationId::NULL,
1269+
/// tskit::IndividualId::NULL)
1270+
/// .unwrap();
1271+
/// tables
1272+
/// .add_node(tskit::TSK_NODE_IS_SAMPLE, 1.0, tskit::PopulationId::NULL,
1273+
/// tskit::IndividualId::NULL)
1274+
/// .unwrap();
1275+
/// let samples = tables.create_node_id_vector(
1276+
/// |row: &tskit::NodeTableRow| row.time > 0.,
1277+
/// );
1278+
/// assert_eq!(samples[0], 1);
1279+
///
1280+
/// // Get all nodes that have a mutation:
1281+
///
1282+
/// // fn node_has_mutation(
1283+
/// // // dyn trait here means this
1284+
/// // // will work with TreeSequence, too.
1285+
/// // tables_type: &dyn std::ops::Deref<Target=tskit::table_views::TableViews>,
1286+
/// // row: &tskit::NodeTableRow,
1287+
/// // ) -> bool {
1288+
/// // for mrow in tables_type.mutations_iter() {
1289+
/// // if mrow.node == row.id {
1290+
/// // return true;
1291+
/// // }
1292+
/// // }
1293+
/// // false
1294+
/// // }
1295+
///
1296+
/// // // Get all nodes that have a mutation:
1297+
///
1298+
/// // tables.add_mutation(0, 0, tskit::MutationId::NULL, 0.0, None).unwrap();
1299+
/// // let samples_with_mut = tables.create_node_id_vector(
1300+
/// // |row: &tskit::NodeTableRow| node_has_mutation(&tables, row));
1301+
/// // assert_eq!(samples_with_mut[0], 0);
1302+
/// ```
1303+
pub fn create_node_id_vector(&self, f: impl FnMut(&crate::NodeTableRow) -> bool) -> Vec<crate::NodeId>;
1304+
}
1305+
}
1306+
};
1307+
}
1308+
11941309
#[cfg(test)]
11951310
mod test {
11961311
use crate::error::TskitError;

src/table_collection.rs

Lines changed: 14 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use std::ops::{Deref, DerefMut};
1+
use delegate::delegate;
22
use std::vec;
33

44
use crate::bindings as ll_bindings;
@@ -76,20 +76,6 @@ impl Drop for TableCollection {
7676
}
7777
}
7878

79-
impl Deref for TableCollection {
80-
type Target = crate::table_views::TableViews;
81-
82-
fn deref(&self) -> &Self::Target {
83-
&self.views
84-
}
85-
}
86-
87-
impl DerefMut for TableCollection {
88-
fn deref_mut(&mut self) -> &mut Self::Target {
89-
&mut self.views
90-
}
91-
}
92-
9379
/// Returns a pointer to an uninitialized tsk_table_collection_t
9480
pub(crate) fn uninit_table_collection() -> MBox<ll_bindings::tsk_table_collection_t> {
9581
let temp = unsafe {
@@ -797,8 +783,10 @@ impl TableCollection {
797783
idmap: bool,
798784
) -> Result<Option<&[NodeId]>, TskitError> {
799785
if idmap {
800-
self.idmap
801-
.resize(usize::try_from(self.nodes().num_rows())?, NodeId::NULL);
786+
self.idmap.resize(
787+
usize::try_from(self.views.nodes().num_rows())?,
788+
NodeId::NULL,
789+
);
802790
}
803791
let rv = unsafe {
804792
ll_bindings::tsk_table_collection_simplify(
@@ -1232,4 +1220,13 @@ impl TableCollection {
12321220
};
12331221
handle_tsk_return_value!(rv)
12341222
}
1223+
1224+
delegate! {
1225+
to self.views {
1226+
/// Get mutable reference to the [``NodeTable``](crate::NodeTable).
1227+
pub fn nodes_mut(&mut self) -> &mut crate::NodeTable;
1228+
}
1229+
}
1230+
1231+
delegate_table_view_api!();
12351232
}

src/table_views.rs

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -222,26 +222,26 @@ impl TableViews {
222222
///
223223
/// // Get all nodes that have a mutation:
224224
///
225-
/// fn node_has_mutation(
226-
/// // dyn trait here means this
227-
/// // will work with TreeSequence, too.
228-
/// tables_type: &dyn std::ops::Deref<Target=tskit::table_views::TableViews>,
229-
/// row: &tskit::NodeTableRow,
230-
/// ) -> bool {
231-
/// for mrow in tables_type.mutations_iter() {
232-
/// if mrow.node == row.id {
233-
/// return true;
234-
/// }
235-
/// }
236-
/// false
237-
/// }
225+
/// // fn node_has_mutation(
226+
/// // // dyn trait here means this
227+
/// // // will work with TreeSequence, too.
228+
/// // tables_type: &dyn std::ops::Deref<Target=tskit::table_views::TableViews>,
229+
/// // row: &tskit::NodeTableRow,
230+
/// // ) -> bool {
231+
/// // for mrow in tables_type.mutations_iter() {
232+
/// // if mrow.node == row.id {
233+
/// // return true;
234+
/// // }
235+
/// // }
236+
/// // false
237+
/// // }
238238
///
239-
/// // Get all nodes that have a mutation:
239+
/// // // Get all nodes that have a mutation:
240240
///
241-
/// tables.add_mutation(0, 0, tskit::MutationId::NULL, 0.0, None).unwrap();
242-
/// let samples_with_mut = tables.create_node_id_vector(
243-
/// |row: &tskit::NodeTableRow| node_has_mutation(&tables, row));
244-
/// assert_eq!(samples_with_mut[0], 0);
241+
/// // tables.add_mutation(0, 0, tskit::MutationId::NULL, 0.0, None).unwrap();
242+
/// // let samples_with_mut = tables.create_node_id_vector(
243+
/// // |row: &tskit::NodeTableRow| node_has_mutation(&tables, row));
244+
/// // assert_eq!(samples_with_mut[0], 0);
245245
/// ```
246246
pub fn create_node_id_vector(
247247
&self,

src/trees.rs

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -209,14 +209,6 @@ impl Drop for TreeSequence {
209209
}
210210
}
211211

212-
impl std::ops::Deref for TreeSequence {
213-
type Target = crate::table_views::TableViews;
214-
215-
fn deref(&self) -> &Self::Target {
216-
&self.views
217-
}
218-
}
219-
220212
impl TreeSequence {
221213
/// Create a tree sequence from a [`TableCollection`].
222214
/// In general, [`TableCollection::tree_sequence`] may be preferred.
@@ -545,6 +537,8 @@ impl TreeSequence {
545537
};
546538
handle_tsk_return_value!(rv, crate::ProvenanceId::from(rv))
547539
}
540+
541+
delegate_table_view_api!();
548542
}
549543

550544
impl TryFrom<TableCollection> for TreeSequence {

0 commit comments

Comments
 (0)