From 15079a23e13590fdddd2c847b18ce14d71198f7e Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Fri, 22 Jul 2022 10:29:28 -0700
Subject: [PATCH 01/22] Draft of natural tree iterator w/o GATS.

---
 src/trees.rs | 88 +++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 81 insertions(+), 7 deletions(-)

diff --git a/src/trees.rs b/src/trees.rs
index 56ce4e2b7..4e107500c 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -1,6 +1,4 @@
-use std::mem::MaybeUninit;
-use std::ops::Deref;
-use std::ops::DerefMut;
+use std::ptr::NonNull;
 
 use crate::bindings as ll_bindings;
 use crate::error::TskitError;
@@ -35,13 +33,89 @@ pub struct Tree {
     advanced: bool,
 }
 
-impl Drop for Tree {
-    fn drop(&mut self) {
-        let rv = unsafe { tsk_tree_free(&mut self.inner) };
-        assert_eq!(rv, 0);
+pub struct TreeIterator {
+    tree: ll_bindings::tsk_tree_t,
+    current_tree: i32,
+    advanced: bool,
+    num_nodes: tsk_size_t,
+    array_len: tsk_size_t,
+    flags: TreeFlags,
+}
+
+impl TreeIterator {
+    fn item(&mut self) -> NonOwningTree {
+        NonOwningTree::new(
+            NonNull::from(&mut self.tree),
+            self.current_tree,
+            self.advanced,
+            self.num_nodes,
+            self.array_len,
+            self.flags,
+        )
+    }
+}
+
+pub struct NonOwningTree {
+    tree: NonNull<ll_bindings::tsk_tree_t>,
+    current_tree: i32,
+    advanced: bool,
+    num_nodes: tsk_size_t,
+    array_len: tsk_size_t,
+    flags: TreeFlags,
+}
+
+impl NonOwningTree {
+    fn new(
+        tree: NonNull<ll_bindings::tsk_tree_t>,
+        current_tree: i32,
+        advanced: bool,
+        num_nodes: tsk_size_t,
+        array_len: tsk_size_t,
+        flags: TreeFlags,
+    ) -> Self {
+        Self {
+            tree,
+            current_tree,
+            advanced,
+            num_nodes,
+            array_len,
+            flags,
+        }
+    }
+}
+
+impl Iterator for TreeIterator {
+    type Item = NonOwningTree;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        let rv = if self.current_tree == 0 {
+            unsafe { ll_bindings::tsk_tree_first(&mut self.tree) }
+        } else {
+            unsafe { ll_bindings::tsk_tree_next(&mut self.tree) }
+        };
+        if rv == 0 {
+            self.advanced = false;
+            self.current_tree += 1;
+        } else if rv == 1 {
+            self.advanced = true;
+            self.current_tree += 1;
+        } else if rv < 0 {
+            panic_on_tskit_error!(rv);
+        }
+        if self.advanced {
+            Some(self.item())
+        } else {
+            None
+        }
     }
 }
 
+// Trait defining iteration over nodes.
+trait NodeIterator {
+    fn next_node(&mut self);
+    fn current_node(&mut self) -> Option<NodeId>;
+}
+
 impl Deref for Tree {
     type Target = TreeInterface;
     fn deref(&self) -> &Self::Target {

From e3b7d262b596a88aeca9efcc65371cfdefb58d9b Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Fri, 22 Jul 2022 10:30:31 -0700
Subject: [PATCH 02/22] Need drop.

---
 src/trees.rs | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/trees.rs b/src/trees.rs
index 4e107500c..fb0782168 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -110,6 +110,12 @@ impl Iterator for TreeIterator {
     }
 }
 
+impl Drop for TreeIterator {
+    fn drop(&mut self) {
+        unsafe { ll_bindings::tsk_tree_free(&mut self.tree) };
+    }
+}
+
 // Trait defining iteration over nodes.
 trait NodeIterator {
     fn next_node(&mut self);

From 9c387f16e777e81ccb1297565a75be1dd163f715 Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Fri, 22 Jul 2022 10:34:57 -0700
Subject: [PATCH 03/22] Constructor

---
 src/trees.rs | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/src/trees.rs b/src/trees.rs
index fb0782168..5e0e4b54f 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -1,3 +1,4 @@
+use std::mem::MaybeUninit;
 use std::ptr::NonNull;
 
 use crate::bindings as ll_bindings;
@@ -43,6 +44,23 @@ pub struct TreeIterator {
 }
 
 impl TreeIterator {
+    // FIXME: init if fallible!
+    fn new(treeseq: &TreeSequence) -> Self {
+        let mut tree = MaybeUninit::<ll_bindings::tsk_tree_t>::uninit();
+        let _rv = unsafe {
+            ll_bindings::tsk_tree_init(tree.as_mut_ptr(), treeseq.inner.as_ref(), 0);
+        };
+        let tree = unsafe { tree.assume_init() };
+
+        Self {
+            tree,
+            current_tree: -1,
+            advanced: false,
+            num_nodes: 0,
+            array_len: 0,
+            flags: 0.into(),
+        }
+    }
     fn item(&mut self) -> NonOwningTree {
         NonOwningTree::new(
             NonNull::from(&mut self.tree),

From 35c10da3b6589433bfddd5aa895c7e1b52e53ab4 Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Fri, 22 Jul 2022 10:40:32 -0700
Subject: [PATCH 04/22] rudimentary test

---
 src/trees.rs | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/trees.rs b/src/trees.rs
index 5e0e4b54f..93f6a1583 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -73,6 +73,7 @@ impl TreeIterator {
     }
 }
 
+#[derive(Debug)]
 pub struct NonOwningTree {
     tree: NonNull<ll_bindings::tsk_tree_t>,
     current_tree: i32,
@@ -439,6 +440,11 @@ impl TreeSequence {
         Ok(tree)
     }
 
+    /// Return an iterator over the trees.
+    pub fn trees(&self) -> impl Iterator<Item = NonOwningTree> + '_ {
+        TreeIterator::new(self)
+    }
+
     /// Get the list of samples as a vector.
     /// # Panics
     ///
@@ -747,6 +753,9 @@ pub(crate) mod test_trees {
             assert_eq!(tree.num_tracked_samples(1.into()).unwrap(), 1);
             assert_eq!(tree.num_tracked_samples(0.into()).unwrap(), 2);
         }
+        for tree in treeseq.trees() {
+            println!("{:?}", tree);
+        }
     }
 
     #[should_panic]

From 0f6d23617dff19ae90563221fc645437d8d1d365 Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Fri, 22 Jul 2022 10:59:40 -0700
Subject: [PATCH 05/22] failing test as reminder for later.

---
 src/trees.rs | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/src/trees.rs b/src/trees.rs
index 93f6a1583..caad8f590 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -101,6 +101,10 @@ impl NonOwningTree {
             flags,
         }
     }
+
+    fn as_owned(&self) -> &Self {
+        self
+    }
 }
 
 impl Iterator for TreeIterator {
@@ -753,9 +757,23 @@ pub(crate) mod test_trees {
             assert_eq!(tree.num_tracked_samples(1.into()).unwrap(), 1);
             assert_eq!(tree.num_tracked_samples(0.into()).unwrap(), 2);
         }
+    }
+
+    #[test]
+    fn test_trees() {
+        let treeseq = treeseq_from_small_table_collection();
         for tree in treeseq.trees() {
             println!("{:?}", tree);
         }
+
+        // This is a safety sticking point:
+        // we cannot collect the iterable itself b/c
+        // the underlying tree memory is re-used.
+        let v = treeseq
+            .trees()
+            .map(|t| t.as_owned()) // this is what we mean, but this is broken
+            .collect::<Vec<NonOwningTree>>();
+        assert_eq!(v.len(), 2);
     }
 
     #[should_panic]

From 32903edfcc536ae602a5843716b94a04e55e8a34 Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Fri, 22 Jul 2022 11:08:26 -0700
Subject: [PATCH 06/22] rename, show that we can get internal arrays out.

---
 src/trees.rs | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/trees.rs b/src/trees.rs
index caad8f590..349a92912 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -760,10 +760,13 @@ pub(crate) mod test_trees {
     }
 
     #[test]
-    fn test_trees() {
+    fn test_new_trees_iterator() {
         let treeseq = treeseq_from_small_table_collection();
         for tree in treeseq.trees() {
-            println!("{:?}", tree);
+            let parents = unsafe {
+                std::slice::from_raw_parts((*tree.tree.as_ptr()).parent as *const NodeId, 4_usize)
+            };
+            println!("{:?}", parents);
         }
 
         // This is a safety sticking point:

From 94d8e128994e447c0a18c74ff7b5d61f3b8f8332 Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Sat, 23 Jul 2022 05:57:40 -0700
Subject: [PATCH 07/22] progress, unimplemented()

---
 src/trees.rs | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/src/trees.rs b/src/trees.rs
index 349a92912..23c3b1b43 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -43,6 +43,16 @@ pub struct TreeIterator {
     flags: TreeFlags,
 }
 
+impl FromIterator<NonOwningTree> for Vec<Tree> {
+    fn from_iter<T: IntoIterator<Item = NonOwningTree>>(iter: T) -> Self {
+        let mut out = vec![];
+        for i in iter {
+            unimplemented!("not yet!");
+        }
+        out
+    }
+}
+
 impl TreeIterator {
     // FIXME: init if fallible!
     fn new(treeseq: &TreeSequence) -> Self {
@@ -772,10 +782,9 @@ pub(crate) mod test_trees {
         // This is a safety sticking point:
         // we cannot collect the iterable itself b/c
         // the underlying tree memory is re-used.
-        let v = treeseq
-            .trees()
-            .map(|t| t.as_owned()) // this is what we mean, but this is broken
-            .collect::<Vec<NonOwningTree>>();
+        let i = treeseq
+            .trees();
+        let v = Vec::<Tree>::from_iter(i);
         assert_eq!(v.len(), 2);
     }
 

From 91722bb28331a1f39bf5ee1cc592ad60cd7c3283 Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Thu, 28 Jul 2022 14:13:00 -0700
Subject: [PATCH 08/22] import fixes

---
 src/trees.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/trees.rs b/src/trees.rs
index 23c3b1b43..79896645c 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -1,4 +1,5 @@
 use std::mem::MaybeUninit;
+use std::ops::{Deref, DerefMut};
 use std::ptr::NonNull;
 
 use crate::bindings as ll_bindings;
@@ -782,8 +783,7 @@ pub(crate) mod test_trees {
         // This is a safety sticking point:
         // we cannot collect the iterable itself b/c
         // the underlying tree memory is re-used.
-        let i = treeseq
-            .trees();
+        let i = treeseq.trees();
         let v = Vec::<Tree>::from_iter(i);
         assert_eq!(v.len(), 2);
     }

From 07319c7d5be1fe36ac9bcb9905cae8b57f2fecfd Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Mon, 26 Sep 2022 08:07:13 -0700
Subject: [PATCH 09/22] fixes after rebase

---
 src/trees.rs | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/trees.rs b/src/trees.rs
index 79896645c..7897664f5 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -1,6 +1,5 @@
 use std::mem::MaybeUninit;
 use std::ops::{Deref, DerefMut};
-use std::ptr::NonNull;
 
 use crate::bindings as ll_bindings;
 use crate::error::TskitError;
@@ -22,7 +21,6 @@ use crate::TreeSequenceFlags;
 use crate::TskReturnValue;
 use crate::TskitTypeAccess;
 use crate::{tsk_id_t, tsk_size_t, TableCollection};
-use ll_bindings::tsk_tree_free;
 use std::ptr::NonNull;
 
 /// A Tree.
@@ -59,7 +57,7 @@ impl TreeIterator {
     fn new(treeseq: &TreeSequence) -> Self {
         let mut tree = MaybeUninit::<ll_bindings::tsk_tree_t>::uninit();
         let _rv = unsafe {
-            ll_bindings::tsk_tree_init(tree.as_mut_ptr(), treeseq.inner.as_ref(), 0);
+            ll_bindings::tsk_tree_init(tree.as_mut_ptr(), treeseq.as_ptr(), 0);
         };
         let tree = unsafe { tree.assume_init() };
 

From 6c769cdfac91e4e75e1f5a758eca8f5651d58e90 Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Mon, 26 Sep 2022 09:19:50 -0700
Subject: [PATCH 10/22] Baby steps towards FromIterator

---
 src/trees.rs | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/trees.rs b/src/trees.rs
index 7897664f5..15a76f9d8 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -46,6 +46,10 @@ impl FromIterator<NonOwningTree> for Vec<Tree> {
     fn from_iter<T: IntoIterator<Item = NonOwningTree>>(iter: T) -> Self {
         let mut out = vec![];
         for i in iter {
+            let mut lltree = MaybeUninit::<ll_bindings::tsk_tree_t>::uninit();
+            // NOTE: the following is fallible, but this function is not meant to be
+            let rv = unsafe { ll_bindings::tsk_tree_copy(i.tree.as_ptr(), lltree.as_mut_ptr(), 0) };
+            assert_eq!(rv, 0);
             unimplemented!("not yet!");
         }
         out

From 898aae9f86ed1669f4908bc9c1ae7f7cf98f3aca Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Tue, 27 Sep 2022 08:24:03 -0700
Subject: [PATCH 11/22] Redesign:

* fn trees() now returns a more generic type.
  * This breaks the ability to collect() the
  iterator Item, which is good, because collection
  cannot easily be made safe due to the C layout.
* We add TreeInterface to NonOwning tree.
  (Had to do this for any of this to be useful.)
---
 src/trees.rs | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/trees.rs b/src/trees.rs
index 15a76f9d8..023402008 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -89,6 +89,7 @@ impl TreeIterator {
 #[derive(Debug)]
 pub struct NonOwningTree {
     tree: NonNull<ll_bindings::tsk_tree_t>,
+    api: TreeInterface,
     current_tree: i32,
     advanced: bool,
     num_nodes: tsk_size_t,
@@ -96,6 +97,14 @@ pub struct NonOwningTree {
     flags: TreeFlags,
 }
 
+impl Deref for NonOwningTree {
+    type Target = TreeInterface;
+
+    fn deref(&self) -> &Self::Target {
+        &self.api
+    }
+}
+
 impl NonOwningTree {
     fn new(
         tree: NonNull<ll_bindings::tsk_tree_t>,
@@ -105,8 +114,10 @@ impl NonOwningTree {
         array_len: tsk_size_t,
         flags: TreeFlags,
     ) -> Self {
+        let api = TreeInterface::new(tree, num_nodes, array_len, flags);
         Self {
             tree,
+            api,
             current_tree,
             advanced,
             num_nodes,
@@ -458,7 +469,7 @@ impl TreeSequence {
     }
 
     /// Return an iterator over the trees.
-    pub fn trees(&self) -> impl Iterator<Item = NonOwningTree> + '_ {
+    pub fn trees(&self) -> impl Iterator<Item = impl Deref<Target = TreeInterface>> + '_ {
         TreeIterator::new(self)
     }
 

From 2589d2af2e8375c8ee879a535a37b3fcdf52d33a Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Tue, 27 Sep 2022 08:35:23 -0700
Subject: [PATCH 12/22] update tests -- they are erroring

---
 src/tree_interface.rs |  1 +
 src/trees.rs          | 15 ++++++++-------
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/tree_interface.rs b/src/tree_interface.rs
index 271102414..7ac16da50 100644
--- a/src/tree_interface.rs
+++ b/src/tree_interface.rs
@@ -10,6 +10,7 @@ use crate::TskitError;
 use crate::TskitTypeAccess;
 use std::ptr::NonNull;
 
+#[derive(Debug)]
 pub struct TreeInterface {
     non_owned_pointer: NonNull<ll_bindings::tsk_tree_t>,
     num_nodes: tsk_size_t,
diff --git a/src/trees.rs b/src/trees.rs
index 023402008..d7ed87c1a 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -787,18 +787,19 @@ pub(crate) mod test_trees {
     fn test_new_trees_iterator() {
         let treeseq = treeseq_from_small_table_collection();
         for tree in treeseq.trees() {
-            let parents = unsafe {
-                std::slice::from_raw_parts((*tree.tree.as_ptr()).parent as *const NodeId, 4_usize)
-            };
-            println!("{:?}", parents);
+            for n in tree.traverse_nodes(NodeTraversalOrder::Preorder) {
+                for p in tree.parents(n).unwrap() {
+                    println!("{:?}", p);
+                }
+            }
         }
 
         // This is a safety sticking point:
         // we cannot collect the iterable itself b/c
         // the underlying tree memory is re-used.
-        let i = treeseq.trees();
-        let v = Vec::<Tree>::from_iter(i);
-        assert_eq!(v.len(), 2);
+        // let i = treeseq.trees();
+        // let v = Vec::<Tree>::from_iter(i);
+        // assert_eq!(v.len(), 2);
     }
 
     #[should_panic]

From 5b3ce1a7e1cf93cfd995d66599fc9fcc1b2f7ca2 Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Tue, 27 Sep 2022 08:40:24 -0700
Subject: [PATCH 13/22] remove FromIterator draft

---
 src/trees.rs | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/src/trees.rs b/src/trees.rs
index d7ed87c1a..6d7cfddd6 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -42,20 +42,6 @@ pub struct TreeIterator {
     flags: TreeFlags,
 }
 
-impl FromIterator<NonOwningTree> for Vec<Tree> {
-    fn from_iter<T: IntoIterator<Item = NonOwningTree>>(iter: T) -> Self {
-        let mut out = vec![];
-        for i in iter {
-            let mut lltree = MaybeUninit::<ll_bindings::tsk_tree_t>::uninit();
-            // NOTE: the following is fallible, but this function is not meant to be
-            let rv = unsafe { ll_bindings::tsk_tree_copy(i.tree.as_ptr(), lltree.as_mut_ptr(), 0) };
-            assert_eq!(rv, 0);
-            unimplemented!("not yet!");
-        }
-        out
-    }
-}
-
 impl TreeIterator {
     // FIXME: init if fallible!
     fn new(treeseq: &TreeSequence) -> Self {

From 21ab719bd983b03d5faa5c433d0cc3d45340231c Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Tue, 27 Sep 2022 08:46:46 -0700
Subject: [PATCH 14/22] check rv of tsk_tree_init

---
 src/trees.rs | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/trees.rs b/src/trees.rs
index 6d7cfddd6..232300afb 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -46,9 +46,8 @@ impl TreeIterator {
     // FIXME: init if fallible!
     fn new(treeseq: &TreeSequence) -> Self {
         let mut tree = MaybeUninit::<ll_bindings::tsk_tree_t>::uninit();
-        let _rv = unsafe {
-            ll_bindings::tsk_tree_init(tree.as_mut_ptr(), treeseq.as_ptr(), 0);
-        };
+        let rv = unsafe { ll_bindings::tsk_tree_init(tree.as_mut_ptr(), treeseq.as_ptr(), 0) };
+        assert_eq!(rv, 0);
         let tree = unsafe { tree.assume_init() };
 
         Self {

From c6bd9ad5203ceba7e267e452214dcf088b435120 Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Tue, 27 Sep 2022 08:48:33 -0700
Subject: [PATCH 15/22] add unimplemented to tell us what to fix later

---
 src/trees.rs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/trees.rs b/src/trees.rs
index 232300afb..2eb927a2b 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -50,6 +50,7 @@ impl TreeIterator {
         assert_eq!(rv, 0);
         let tree = unsafe { tree.assume_init() };
 
+        unimplemented!("need to handle num_nodes, etc...");
         Self {
             tree,
             current_tree: -1,

From d08dbc64d1dde866ad903bfcd37a494ddd829e62 Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Tue, 27 Sep 2022 09:02:39 -0700
Subject: [PATCH 16/22] properly initialize the tree iterator -- tests now pass

---
 src/trees.rs | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/trees.rs b/src/trees.rs
index 2eb927a2b..c136c315c 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -44,19 +44,18 @@ pub struct TreeIterator {
 
 impl TreeIterator {
     // FIXME: init if fallible!
-    fn new(treeseq: &TreeSequence) -> Self {
+    fn new(treeseq: &TreeSequence, num_nodes: u64) -> Self {
         let mut tree = MaybeUninit::<ll_bindings::tsk_tree_t>::uninit();
         let rv = unsafe { ll_bindings::tsk_tree_init(tree.as_mut_ptr(), treeseq.as_ptr(), 0) };
         assert_eq!(rv, 0);
         let tree = unsafe { tree.assume_init() };
 
-        unimplemented!("need to handle num_nodes, etc...");
         Self {
             tree,
             current_tree: -1,
             advanced: false,
-            num_nodes: 0,
-            array_len: 0,
+            num_nodes,
+            array_len: num_nodes + 1,
             flags: 0.into(),
         }
     }
@@ -456,7 +455,9 @@ impl TreeSequence {
 
     /// Return an iterator over the trees.
     pub fn trees(&self) -> impl Iterator<Item = impl Deref<Target = TreeInterface>> + '_ {
-        TreeIterator::new(self)
+        // TODO: should have a safe wrapper for this
+        let num_nodes = unsafe { (*(*self.as_ptr()).tables).nodes.num_rows };
+        TreeIterator::new(self, num_nodes)
     }
 
     /// Get the list of samples as a vector.

From 5a3bd0a697b706c5c73404655d734d970016a8c2 Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Tue, 27 Sep 2022 10:14:47 -0700
Subject: [PATCH 17/22] valgrind tests now pass

---
 src/trees.rs | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/trees.rs b/src/trees.rs
index c136c315c..2b4f91020 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -161,6 +161,12 @@ impl Deref for Tree {
     }
 }
 
+impl Drop for Tree {
+    fn drop(&mut self) {
+        let rv = unsafe { ll_bindings::tsk_tree_free(&mut self.inner) };
+    }
+}
+
 impl DerefMut for Tree {
     fn deref_mut(&mut self) -> &mut Self::Target {
         &mut self.api

From 4069567be929cdee683b3d2f194ae4eb1d53579a Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Tue, 27 Sep 2022 10:16:55 -0700
Subject: [PATCH 18/22] use rv from drop

---
 src/trees.rs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/trees.rs b/src/trees.rs
index 2b4f91020..c7117e249 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -164,6 +164,7 @@ impl Deref for Tree {
 impl Drop for Tree {
     fn drop(&mut self) {
         let rv = unsafe { ll_bindings::tsk_tree_free(&mut self.inner) };
+        assert_eq!(rv, 0);
     }
 }
 

From 190d83cc4ec2d63d113bfc8584b2a45e9d0df446 Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Tue, 27 Sep 2022 10:18:29 -0700
Subject: [PATCH 19/22] clippy lints

---
 src/trees.rs | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/src/trees.rs b/src/trees.rs
index c7117e249..76b1ac2e6 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -62,8 +62,6 @@ impl TreeIterator {
     fn item(&mut self) -> NonOwningTree {
         NonOwningTree::new(
             NonNull::from(&mut self.tree),
-            self.current_tree,
-            self.advanced,
             self.num_nodes,
             self.array_len,
             self.flags,
@@ -73,13 +71,7 @@ impl TreeIterator {
 
 #[derive(Debug)]
 pub struct NonOwningTree {
-    tree: NonNull<ll_bindings::tsk_tree_t>,
     api: TreeInterface,
-    current_tree: i32,
-    advanced: bool,
-    num_nodes: tsk_size_t,
-    array_len: tsk_size_t,
-    flags: TreeFlags,
 }
 
 impl Deref for NonOwningTree {
@@ -93,27 +85,15 @@ impl Deref for NonOwningTree {
 impl NonOwningTree {
     fn new(
         tree: NonNull<ll_bindings::tsk_tree_t>,
-        current_tree: i32,
-        advanced: bool,
         num_nodes: tsk_size_t,
         array_len: tsk_size_t,
         flags: TreeFlags,
     ) -> Self {
         let api = TreeInterface::new(tree, num_nodes, array_len, flags);
         Self {
-            tree,
             api,
-            current_tree,
-            advanced,
-            num_nodes,
-            array_len,
-            flags,
         }
     }
-
-    fn as_owned(&self) -> &Self {
-        self
-    }
 }
 
 impl Iterator for TreeIterator {

From c64e6ca5ff4b7c8e1c4bdd1c88d6d5dc1d731a48 Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Tue, 27 Sep 2022 10:23:27 -0700
Subject: [PATCH 20/22] NonOwningTree is repr(transparent)

---
 src/trees.rs | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/trees.rs b/src/trees.rs
index 76b1ac2e6..db5fafe20 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -70,6 +70,7 @@ impl TreeIterator {
 }
 
 #[derive(Debug)]
+#[repr(transparent)]
 pub struct NonOwningTree {
     api: TreeInterface,
 }
@@ -90,9 +91,7 @@ impl NonOwningTree {
         flags: TreeFlags,
     ) -> Self {
         let api = TreeInterface::new(tree, num_nodes, array_len, flags);
-        Self {
-            api,
-        }
+        Self { api }
     }
 }
 

From b6b5e121dc0615b22758d3b9ac19a5fdfc832655 Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Tue, 27 Sep 2022 10:34:40 -0700
Subject: [PATCH 21/22] we still have the irritating issue w/collection

---
 src/trees.rs | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/trees.rs b/src/trees.rs
index db5fafe20..59064eccc 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -770,9 +770,17 @@ pub(crate) mod test_trees {
         // This is a safety sticking point:
         // we cannot collect the iterable itself b/c
         // the underlying tree memory is re-used.
-        // let i = treeseq.trees();
-        // let v = Vec::<Tree>::from_iter(i);
-        // assert_eq!(v.len(), 2);
+        let i = treeseq.trees();
+        let v = Vec::<_>::from_iter(i);
+        assert_eq!(v.len(), 2);
+        for i in v {
+            println!("{:?}", i.parent_array());
+        }
+
+        let v = treeseq.trees().collect::<Vec<_>>();
+        for i in v {
+            println!("{:?}", i.parent_array());
+        }
     }
 
     #[should_panic]

From 2e0b53283df987a4018734b6f1a1de264b21b72d Mon Sep 17 00:00:00 2001
From: "Kevin R. Thornton" <krthornt@uci.edu>
Date: Tue, 27 Sep 2022 10:42:47 -0700
Subject: [PATCH 22/22] panic on the sticking point

---
 src/trees.rs | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/trees.rs b/src/trees.rs
index 59064eccc..d38040b99 100644
--- a/src/trees.rs
+++ b/src/trees.rs
@@ -767,6 +767,10 @@ pub(crate) mod test_trees {
             }
         }
 
+        // NOTE: the following blocks make cargo valgrind crash,
+        // which is a sign of badness.
+        panic!("cargo valgrind will fail here");
+
         // This is a safety sticking point:
         // we cannot collect the iterable itself b/c
         // the underlying tree memory is re-used.