Skip to content

Commit 972af66

Browse files
molpopgenmolpopgen
authored
refactor: mark internal fn for column access unsafe (#712)
1 parent ead316c commit 972af66

File tree

11 files changed

+408
-161
lines changed

11 files changed

+408
-161
lines changed

‎src/_macros.rs‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -68,13 +68,20 @@ macro_rules! handle_metadata_return {
6868
macro_rules! raw_metadata_getter_for_tables {
6969
($idtype: ty) => {
7070
fn raw_metadata<I: Into<$idtype>>(&self, row: I) -> Option<&[u8]> {
71-
$crate::sys::tsk_ragged_column_access::<'_, u8, $idtype, _, _>(
72-
row.into(),
73-
self.as_ref().metadata,
74-
self.num_rows(),
75-
self.as_ref().metadata_offset,
76-
self.as_ref().metadata_length,
77-
)
71+
assert!(
72+
(self.num_rows() == 0 && self.as_ref().metadata_length == 0)
73+
|| (!self.as_ref().metadata.is_null()
74+
&& !self.as_ref().metadata_offset.is_null())
75+
);
76+
unsafe {
77+
$crate::sys::tsk_ragged_column_access::<'_, u8, $idtype, _, _>(
78+
row.into(),
79+
self.as_ref().metadata,
80+
self.num_rows(),
81+
self.as_ref().metadata_offset,
82+
self.as_ref().metadata_length,
83+
)
84+
}
7885
}
7986
};
8087
}

‎src/edge_table.rs‎

Lines changed: 36 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,15 @@ impl EdgeTable {
223223
/// * `Some(parent)` if `u` is valid.
224224
/// * `None` otherwise.
225225
pub fn parent<E: Into<EdgeId> + Copy>(&self, row: E) -> Option<NodeId> {
226-
sys::tsk_column_access::<NodeId, _, _, _>(row.into(), self.as_ref().parent, self.num_rows())
226+
assert!(self.num_rows() == 0 || !self.as_ref().parent.is_null());
227+
// SAFETY: either the column is empty or the point is not NULL
228+
unsafe {
229+
sys::tsk_column_access::<NodeId, _, _, _>(
230+
row.into(),
231+
self.as_ref().parent,
232+
self.num_rows(),
233+
)
234+
}
227235
}
228236

229237
/// Return the ``child`` value from row ``row`` of the table.
@@ -233,7 +241,15 @@ impl EdgeTable {
233241
/// * `Some(child)` if `u` is valid.
234242
/// * `None` otherwise.
235243
pub fn child<E: Into<EdgeId> + Copy>(&self, row: E) -> Option<NodeId> {
236-
sys::tsk_column_access::<NodeId, _, _, _>(row.into(), self.as_ref().child, self.num_rows())
244+
assert!(self.num_rows() == 0 || !self.as_ref().child.is_null());
245+
// SAFETY: either the column is empty or the point is not NULL
246+
unsafe {
247+
sys::tsk_column_access::<NodeId, _, _, _>(
248+
row.into(),
249+
self.as_ref().child,
250+
self.num_rows(),
251+
)
252+
}
237253
}
238254

239255
/// Return the ``left`` value from row ``row`` of the table.
@@ -243,7 +259,15 @@ impl EdgeTable {
243259
/// * `Some(position)` if `u` is valid.
244260
/// * `None` otherwise.
245261
pub fn left<E: Into<EdgeId> + Copy>(&self, row: E) -> Option<Position> {
246-
sys::tsk_column_access::<Position, _, _, _>(row.into(), self.as_ref().left, self.num_rows())
262+
assert!(self.num_rows() == 0 || !self.as_ref().left.is_null());
263+
// SAFETY: either the column is empty or the point is not NULL
264+
unsafe {
265+
sys::tsk_column_access::<Position, _, _, _>(
266+
row.into(),
267+
self.as_ref().left,
268+
self.num_rows(),
269+
)
270+
}
247271
}
248272

249273
/// Return the ``right`` value from row ``row`` of the table.
@@ -253,11 +277,15 @@ impl EdgeTable {
253277
/// * `Some(position)` if `u` is valid.
254278
/// * `None` otherwise.
255279
pub fn right<E: Into<EdgeId> + Copy>(&self, row: E) -> Option<Position> {
256-
sys::tsk_column_access::<Position, _, _, _>(
257-
row.into(),
258-
self.as_ref().right,
259-
self.num_rows(),
260-
)
280+
assert!(self.num_rows() == 0 || !self.as_ref().right.is_null());
281+
// SAFETY: either the column is empty or the point is not NULL
282+
unsafe {
283+
sys::tsk_column_access::<Position, _, _, _>(
284+
row.into(),
285+
self.as_ref().right,
286+
self.num_rows(),
287+
)
288+
}
261289
}
262290

263291
/// Retrieve decoded metadata for a `row`.

‎src/individual_table.rs‎

Lines changed: 35 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -209,11 +209,15 @@ impl IndividualTable {
209209
/// * `Some(flags)` if `row` is valid.
210210
/// * `None` otherwise.
211211
pub fn flags<I: Into<IndividualId> + Copy>(&self, row: I) -> Option<IndividualFlags> {
212-
sys::tsk_column_access::<IndividualFlags, _, _, _>(
213-
row.into(),
214-
self.as_ref().flags,
215-
self.num_rows(),
216-
)
212+
assert!(self.num_rows() == 0 || !self.as_ref().flags.is_null());
213+
// SAFETY: either the column is empty or the point is not NULL
214+
unsafe {
215+
sys::tsk_column_access::<IndividualFlags, _, _, _>(
216+
row.into(),
217+
self.as_ref().flags,
218+
self.num_rows(),
219+
)
220+
}
217221
}
218222

219223
/// Return the locations for a given row.
@@ -223,13 +227,19 @@ impl IndividualTable {
223227
/// * `Some(location)` if `row` is valid.
224228
/// * `None` otherwise.
225229
pub fn location<I: Into<IndividualId> + Copy>(&self, row: I) -> Option<&[Location]> {
226-
sys::tsk_ragged_column_access(
227-
row.into(),
228-
self.as_ref().location,
229-
self.num_rows(),
230-
self.as_ref().location_offset,
231-
self.as_ref().location_length,
232-
)
230+
assert!(
231+
(self.num_rows() == 0 && self.as_ref().location_length == 0)
232+
|| (!self.as_ref().location.is_null() && !self.as_ref().location_offset.is_null())
233+
);
234+
unsafe {
235+
sys::tsk_ragged_column_access(
236+
row.into(),
237+
self.as_ref().location,
238+
self.num_rows(),
239+
self.as_ref().location_offset,
240+
self.as_ref().location_length,
241+
)
242+
}
233243
}
234244

235245
/// Return the parents for a given row.
@@ -239,13 +249,19 @@ impl IndividualTable {
239249
/// * `Some(parents)` if `row` is valid.
240250
/// * `None` otherwise.
241251
pub fn parents<I: Into<IndividualId> + Copy>(&self, row: I) -> Option<&[IndividualId]> {
242-
sys::tsk_ragged_column_access(
243-
row.into(),
244-
self.as_ref().parents,
245-
self.num_rows(),
246-
self.as_ref().parents_offset,
247-
self.as_ref().parents_length,
248-
)
252+
assert!(
253+
(self.num_rows() == 0 && self.as_ref().parents_length == 0)
254+
|| (!self.as_ref().parents.is_null() && !self.as_ref().location_offset.is_null())
255+
);
256+
unsafe {
257+
sys::tsk_ragged_column_access(
258+
row.into(),
259+
self.as_ref().parents,
260+
self.num_rows(),
261+
self.as_ref().parents_offset,
262+
self.as_ref().parents_length,
263+
)
264+
}
249265
}
250266

251267
/// Return the metadata for a given row.

‎src/migration_table.rs‎

Lines changed: 56 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,16 @@ impl MigrationTable {
237237
/// * `Some(position)` if `row` is valid.
238238
/// * `None` otherwise.
239239
pub fn left<M: Into<MigrationId> + Copy>(&self, row: M) -> Option<Position> {
240-
sys::tsk_column_access::<Position, _, _, _>(row.into(), self.as_ref().left, self.num_rows())
240+
assert!(self.num_rows() == 0 || !self.as_ref().time.is_null());
241+
// SAFETY: either the column is empty or the pointer is not null,
242+
// in which case the correct lengths are from the low-level objects
243+
unsafe {
244+
sys::tsk_column_access::<Position, _, _, _>(
245+
row.into(),
246+
self.as_ref().left,
247+
self.num_rows(),
248+
)
249+
}
241250
}
242251

243252
/// Return the right coordinate for a given row.
@@ -247,11 +256,16 @@ impl MigrationTable {
247256
/// * `Some(positions)` if `row` is valid.
248257
/// * `None` otherwise.
249258
pub fn right<M: Into<MigrationId> + Copy>(&self, row: M) -> Option<Position> {
250-
sys::tsk_column_access::<Position, _, _, _>(
251-
row.into(),
252-
self.as_ref().right,
253-
self.num_rows(),
254-
)
259+
assert!(self.num_rows() == 0 || !self.as_ref().right.is_null());
260+
// SAFETY: either the column is empty or the pointer is not null,
261+
// in which case the correct lengths are from the low-level objects
262+
unsafe {
263+
sys::tsk_column_access::<Position, _, _, _>(
264+
row.into(),
265+
self.as_ref().right,
266+
self.num_rows(),
267+
)
268+
}
255269
}
256270

257271
/// Return the node for a given row.
@@ -261,7 +275,16 @@ impl MigrationTable {
261275
/// * `Some(node)` if `row` is valid.
262276
/// * `None` otherwise.
263277
pub fn node<M: Into<MigrationId> + Copy>(&self, row: M) -> Option<NodeId> {
264-
sys::tsk_column_access::<NodeId, _, _, _>(row.into(), self.as_ref().node, self.num_rows())
278+
assert!(self.num_rows() == 0 || !self.as_ref().node.is_null());
279+
// SAFETY: either the column is empty or the pointer is not null,
280+
// in which case the correct lengths are from the low-level objects
281+
unsafe {
282+
sys::tsk_column_access::<NodeId, _, _, _>(
283+
row.into(),
284+
self.as_ref().node,
285+
self.num_rows(),
286+
)
287+
}
265288
}
266289

267290
/// Return the source population for a given row.
@@ -271,11 +294,16 @@ impl MigrationTable {
271294
/// * `Some(population)` if `row` is valid.
272295
/// * `None` otherwise.
273296
pub fn source<M: Into<MigrationId> + Copy>(&self, row: M) -> Option<PopulationId> {
274-
sys::tsk_column_access::<PopulationId, _, _, _>(
275-
row.into(),
276-
self.as_ref().source,
277-
self.num_rows(),
278-
)
297+
assert!(self.num_rows() == 0 || !self.as_ref().time.is_null());
298+
// SAFETY: either the column is empty or the pointer is not null,
299+
// in which case the correct lengths are from the low-level objects
300+
unsafe {
301+
sys::tsk_column_access::<PopulationId, _, _, _>(
302+
row.into(),
303+
self.as_ref().source,
304+
self.num_rows(),
305+
)
306+
}
279307
}
280308

281309
/// Return the destination population for a given row.
@@ -285,11 +313,16 @@ impl MigrationTable {
285313
/// * `Some(population)` if `row` is valid.
286314
/// * `None` otherwise.
287315
pub fn dest<M: Into<MigrationId> + Copy>(&self, row: M) -> Option<PopulationId> {
288-
sys::tsk_column_access::<PopulationId, _, _, _>(
289-
row.into(),
290-
self.as_ref().dest,
291-
self.num_rows(),
292-
)
316+
assert!(self.num_rows() == 0 || !self.as_ref().dest.is_null());
317+
// SAFETY: either the column is empty or the pointer is not null,
318+
// in which case the correct lengths are from the low-level objects
319+
unsafe {
320+
sys::tsk_column_access::<PopulationId, _, _, _>(
321+
row.into(),
322+
self.as_ref().dest,
323+
self.num_rows(),
324+
)
325+
}
293326
}
294327

295328
/// Return the time of the migration event for a given row.
@@ -299,7 +332,12 @@ impl MigrationTable {
299332
/// * `Some(time)` if `row` is valid.
300333
/// * `None` otherwise.
301334
pub fn time<M: Into<MigrationId> + Copy>(&self, row: M) -> Option<Time> {
302-
sys::tsk_column_access::<Time, _, _, _>(row.into(), self.as_ref().time, self.num_rows())
335+
assert!(self.num_rows() == 0 || !self.as_ref().time.is_null());
336+
// SAFETY: either the column is empty or the pointer is not null,
337+
// in which case the correct lengths are from the low-level objects
338+
unsafe {
339+
sys::tsk_column_access::<Time, _, _, _>(row.into(), self.as_ref().time, self.num_rows())
340+
}
303341
}
304342

305343
/// Retrieve decoded metadata for a `row`.

‎src/mutation_table.rs‎

Lines changed: 52 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,16 @@ impl MutationTable {
233233
/// Will return [``IndexError``](crate::TskitError::IndexError)
234234
/// if ``row`` is out of range.
235235
pub fn site<M: Into<MutationId> + Copy>(&self, row: M) -> Option<SiteId> {
236-
sys::tsk_column_access::<SiteId, _, _, _>(row.into(), self.as_ref().site, self.num_rows())
236+
assert!(self.num_rows() == 0 || !self.as_ref().site.is_null());
237+
// SAFETY: either the column is empty or the pointer is not null,
238+
// in which case the correct lengths are from the low-level objects
239+
unsafe {
240+
sys::tsk_column_access::<SiteId, _, _, _>(
241+
row.into(),
242+
self.as_ref().site,
243+
self.num_rows(),
244+
)
245+
}
237246
}
238247

239248
/// Return the ``node`` value from row ``row`` of the table.
@@ -243,7 +252,16 @@ impl MutationTable {
243252
/// Will return [``IndexError``](crate::TskitError::IndexError)
244253
/// if ``row`` is out of range.
245254
pub fn node<M: Into<MutationId> + Copy>(&self, row: M) -> Option<NodeId> {
246-
sys::tsk_column_access::<NodeId, _, _, _>(row.into(), self.as_ref().node, self.num_rows())
255+
assert!(self.num_rows() == 0 || !self.as_ref().node.is_null());
256+
// SAFETY: either the column is empty or the pointer is not null,
257+
// in which case the correct lengths are from the low-level objects
258+
unsafe {
259+
sys::tsk_column_access::<NodeId, _, _, _>(
260+
row.into(),
261+
self.as_ref().node,
262+
self.num_rows(),
263+
)
264+
}
247265
}
248266

249267
/// Return the ``parent`` value from row ``row`` of the table.
@@ -253,11 +271,16 @@ impl MutationTable {
253271
/// Will return [``IndexError``](crate::TskitError::IndexError)
254272
/// if ``row`` is out of range.
255273
pub fn parent<M: Into<MutationId> + Copy>(&self, row: M) -> Option<MutationId> {
256-
sys::tsk_column_access::<MutationId, _, _, _>(
257-
row.into(),
258-
self.as_ref().parent,
259-
self.num_rows(),
260-
)
274+
assert!(self.num_rows() == 0 || !self.as_ref().parent.is_null());
275+
// SAFETY: either the column is empty or the pointer is not null,
276+
// in which case the correct lengths are from the low-level objects
277+
unsafe {
278+
sys::tsk_column_access::<MutationId, _, _, _>(
279+
row.into(),
280+
self.as_ref().parent,
281+
self.num_rows(),
282+
)
283+
}
261284
}
262285

263286
/// Return the ``time`` value from row ``row`` of the table.
@@ -267,7 +290,12 @@ impl MutationTable {
267290
/// Will return [``IndexError``](crate::TskitError::IndexError)
268291
/// if ``row`` is out of range.
269292
pub fn time<M: Into<MutationId> + Copy>(&self, row: M) -> Option<Time> {
270-
sys::tsk_column_access::<Time, _, _, _>(row.into(), self.as_ref().time, self.num_rows())
293+
assert!(self.num_rows() == 0 || !self.as_ref().time.is_null());
294+
// SAFETY: either the column is empty or the pointer is not null,
295+
// in which case the correct lengths are from the low-level objects
296+
unsafe {
297+
sys::tsk_column_access::<Time, _, _, _>(row.into(), self.as_ref().time, self.num_rows())
298+
}
271299
}
272300

273301
/// Get the ``derived_state`` value from row ``row`` of the table.
@@ -281,13 +309,22 @@ impl MutationTable {
281309
/// Will return [``IndexError``](crate::TskitError::IndexError)
282310
/// if ``row`` is out of range.
283311
pub fn derived_state<M: Into<MutationId>>(&self, row: M) -> Option<&[u8]> {
284-
sys::tsk_ragged_column_access(
285-
row.into(),
286-
self.as_ref().derived_state,
287-
self.num_rows(),
288-
self.as_ref().derived_state_offset,
289-
self.as_ref().derived_state_length,
290-
)
312+
assert!(
313+
(self.num_rows() == 0 && self.as_ref().derived_state_length == 0)
314+
|| (!self.as_ref().derived_state.is_null()
315+
&& !self.as_ref().derived_state_offset.is_null())
316+
);
317+
// SAFETY: either both columns are empty or both pointers at not NULL,
318+
// in which case the correct lengths are from the low-level objects
319+
unsafe {
320+
sys::tsk_ragged_column_access(
321+
row.into(),
322+
self.as_ref().derived_state,
323+
self.num_rows(),
324+
self.as_ref().derived_state_offset,
325+
self.as_ref().derived_state_length,
326+
)
327+
}
291328
}
292329

293330
/// Retrieve decoded metadata for a `row`.

0 commit comments

Comments
 (0)