From 9c416dc2b1bf2dc5ede822a63a542ad94c8cb53f Mon Sep 17 00:00:00 2001
From: Markus Westerlind <markus.westerlind@distilnetworks.com>
Date: Tue, 18 Feb 2020 16:11:37 +0100
Subject: [PATCH 01/18] feat: Allow the UndoLog to supplied separately

From https://github.com/rust-lang/rust/pull/69218 it was found that
keeping an individual undo log for each type that need snapshotting
incur noticeable overhead given how often snapshots are done and that
they are in many cases empty.

By separating the log like this, it is possible to store the undo log
and storage separately and only put the together when acting the the
underlying `UnificationTable` or `SnapshotVec`
---
 src/lib.rs               |   1 +
 src/snapshot_vec.rs      | 271 +++++++++++++++++++++++----------------
 src/undo_log.rs          | 207 ++++++++++++++++++++++++++++++
 src/unify/backing_vec.rs | 146 ++++++++++++---------
 src/unify/mod.rs         |  95 ++++++++------
 src/unify/tests.rs       |  18 ++-
 6 files changed, 519 insertions(+), 219 deletions(-)
 create mode 100644 src/undo_log.rs

diff --git a/src/lib.rs b/src/lib.rs
index 233ecf9..aed3524 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -20,4 +20,5 @@ extern crate log;
 extern crate dogged;
 
 pub mod snapshot_vec;
+pub mod undo_log;
 pub mod unify;
diff --git a/src/snapshot_vec.rs b/src/snapshot_vec.rs
index 98088b5..b667abe 100644
--- a/src/snapshot_vec.rs
+++ b/src/snapshot_vec.rs
@@ -22,9 +22,12 @@
 use self::UndoLog::*;
 
 use std::fmt;
+use std::marker::PhantomData;
 use std::mem;
 use std::ops;
 
+use undo_log::{Rollback, Snapshots, UndoLogs, VecLog};
+
 #[derive(Debug)]
 pub enum UndoLog<D: SnapshotVecDelegate> {
     /// New variable with given index was created.
@@ -37,33 +40,92 @@ pub enum UndoLog<D: SnapshotVecDelegate> {
     Other(D::Undo),
 }
 
-pub struct SnapshotVec<D: SnapshotVecDelegate> {
-    values: Vec<D::Value>,
-    undo_log: Vec<UndoLog<D>>,
-    num_open_snapshots: usize,
+impl<D: SnapshotVecDelegate> Rollback<UndoLog<D>> for Vec<D::Value> {
+    fn reverse(&mut self, undo: UndoLog<D>) {
+        match undo {
+            NewElem(i) => {
+                self.pop();
+                assert!(Vec::len(self) == i);
+            }
+
+            SetElem(i, v) => {
+                self[i] = v;
+            }
+
+            Other(u) => {
+                D::reverse(self, u);
+            }
+        }
+    }
+}
+
+pub trait VecLike<D>: AsRef<[D::Value]> + AsMut<[D::Value]> + Rollback<UndoLog<D>>
+where
+    D: SnapshotVecDelegate,
+{
+    fn push(&mut self, item: D::Value);
+    fn len(&self) -> usize;
+    fn reserve(&mut self, size: usize);
+}
+
+impl<D> VecLike<D> for Vec<D::Value>
+where
+    D: SnapshotVecDelegate,
+{
+    fn push(&mut self, item: D::Value) {
+        Vec::push(self, item)
+    }
+    fn len(&self) -> usize {
+        Vec::len(self)
+    }
+    fn reserve(&mut self, size: usize) {
+        Vec::reserve(self, size)
+    }
+}
+
+impl<D> VecLike<D> for &'_ mut Vec<D::Value>
+where
+    D: SnapshotVecDelegate,
+{
+    fn push(&mut self, item: D::Value) {
+        Vec::push(self, item)
+    }
+    fn len(&self) -> usize {
+        Vec::len(self)
+    }
+    fn reserve(&mut self, size: usize) {
+        Vec::reserve(self, size)
+    }
+}
+
+pub struct SnapshotVec<
+    D: SnapshotVecDelegate,
+    V: VecLike<D> = Vec<<D as SnapshotVecDelegate>::Value>,
+    L = VecLog<UndoLog<D>>,
+> {
+    values: V,
+    undo_log: L,
+    _marker: PhantomData<D>,
 }
 
-impl<D> fmt::Debug for SnapshotVec<D>
-    where D: SnapshotVecDelegate,
-          D: fmt::Debug,
-          D::Undo: fmt::Debug,
-          D::Value: fmt::Debug
+impl<D, V, L> fmt::Debug for SnapshotVec<D, V, L>
+where
+    D: SnapshotVecDelegate,
+    V: VecLike<D> + fmt::Debug,
+    L: fmt::Debug,
 {
     fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
         fmt.debug_struct("SnapshotVec")
             .field("values", &self.values)
             .field("undo_log", &self.undo_log)
-            .field("num_open_snapshots", &self.num_open_snapshots)
             .finish()
     }
 }
 
 // Snapshots are tokens that should be created/consumed linearly.
-pub struct Snapshot {
-    // Number of values at the time the snapshot was taken.
+pub struct Snapshot<S = ::undo_log::Snapshot> {
     pub(crate) value_count: usize,
-    // Length of the undo log at the time the snapshot was taken.
-    undo_len: usize,
+    snapshot: S,
 }
 
 pub trait SnapshotVecDelegate {
@@ -74,31 +136,68 @@ pub trait SnapshotVecDelegate {
 }
 
 // HACK(eddyb) manual impl avoids `Default` bound on `D`.
-impl<D: SnapshotVecDelegate> Default for SnapshotVec<D> {
+impl<D: SnapshotVecDelegate, V: VecLike<D> + Default, L: Default> Default for SnapshotVec<D, V, L> {
     fn default() -> Self {
         SnapshotVec {
-            values: Vec::new(),
-            undo_log: Vec::new(),
-            num_open_snapshots: 0,
+            values: V::default(),
+            undo_log: Default::default(),
+            _marker: PhantomData,
         }
     }
 }
 
-impl<D: SnapshotVecDelegate> SnapshotVec<D> {
+impl<D: SnapshotVecDelegate, V: VecLike<D> + Default, L: Default> SnapshotVec<D, V, L> {
     pub fn new() -> Self {
         Self::default()
     }
+}
+
+impl<D: SnapshotVecDelegate, V: VecLike<D>, L> SnapshotVec<D, V, L> {
+    pub fn with_log(values: V, undo_log: L) -> Self {
+        SnapshotVec {
+            values,
+            undo_log,
+            _marker: PhantomData,
+        }
+    }
+}
 
-    pub fn with_capacity(c: usize) -> SnapshotVec<D> {
+impl<D: SnapshotVecDelegate, L: Default> SnapshotVec<D, Vec<D::Value>, L> {
+    pub fn with_capacity(c: usize) -> Self {
         SnapshotVec {
             values: Vec::with_capacity(c),
-            undo_log: Vec::new(),
-            num_open_snapshots: 0,
+            undo_log: Default::default(),
+            _marker: PhantomData,
         }
     }
+}
+
+impl<V: VecLike<D>, D: SnapshotVecDelegate, U> SnapshotVec<D, V, U> {
+    pub fn len(&self) -> usize {
+        self.values.len()
+    }
+
+    pub fn get(&self, index: usize) -> &D::Value {
+        &self.values.as_ref()[index]
+    }
+
+    /// Returns a mutable pointer into the vec; whatever changes you make here cannot be undone
+    /// automatically, so you should be sure call `record()` with some sort of suitable undo
+    /// action.
+    pub fn get_mut(&mut self, index: usize) -> &mut D::Value {
+        &mut self.values.as_mut()[index]
+    }
+
+    /// Reserve space for new values, just like an ordinary vec.
+    pub fn reserve(&mut self, additional: usize) {
+        // This is not affected by snapshots or anything.
+        self.values.reserve(additional);
+    }
+}
 
+impl<V: VecLike<D>, D: SnapshotVecDelegate, L: UndoLogs<UndoLog<D>>> SnapshotVec<D, V, L> {
     fn in_snapshot(&self) -> bool {
-        self.num_open_snapshots > 0
+        self.undo_log.in_snapshot()
     }
 
     pub fn record(&mut self, action: D::Undo) {
@@ -107,10 +206,6 @@ impl<D: SnapshotVecDelegate> SnapshotVec<D> {
         }
     }
 
-    pub fn len(&self) -> usize {
-        self.values.len()
-    }
-
     pub fn push(&mut self, elem: D::Value) -> usize {
         let len = self.values.len();
         self.values.push(elem);
@@ -122,28 +217,11 @@ impl<D: SnapshotVecDelegate> SnapshotVec<D> {
         len
     }
 
-    pub fn get(&self, index: usize) -> &D::Value {
-        &self.values[index]
-    }
-
-    /// Reserve space for new values, just like an ordinary vec.
-    pub fn reserve(&mut self, additional: usize) {
-        // This is not affected by snapshots or anything.
-        self.values.reserve(additional);
-    }
-
-    /// Returns a mutable pointer into the vec; whatever changes you make here cannot be undone
-    /// automatically, so you should be sure call `record()` with some sort of suitable undo
-    /// action.
-    pub fn get_mut(&mut self, index: usize) -> &mut D::Value {
-        &mut self.values[index]
-    }
-
     /// Updates the element at the given index. The old value will saved (and perhaps restored) if
     /// a snapshot is active.
     pub fn set(&mut self, index: usize, new_elem: D::Value) {
-        let old_elem = mem::replace(&mut self.values[index], new_elem);
-        if self.in_snapshot() {
+        let old_elem = mem::replace(&mut self.values.as_mut()[index], new_elem);
+        if self.undo_log.in_snapshot() {
             self.undo_log.push(SetElem(index, old_elem));
         }
     }
@@ -151,8 +229,8 @@ impl<D: SnapshotVecDelegate> SnapshotVec<D> {
     /// Updates all elements. Potentially more efficient -- but
     /// otherwise equivalent to -- invoking `set` for each element.
     pub fn set_all(&mut self, mut new_elems: impl FnMut(usize) -> D::Value) {
-        if !self.in_snapshot() {
-            for (index, slot) in self.values.iter_mut().enumerate() {
+        if !self.undo_log.in_snapshot() {
+            for (index, slot) in self.values.as_mut().iter_mut().enumerate() {
                 *slot = new_elems(index);
             }
         } else {
@@ -167,102 +245,72 @@ impl<D: SnapshotVecDelegate> SnapshotVec<D> {
         OP: FnOnce(&mut D::Value),
         D::Value: Clone,
     {
-        if self.in_snapshot() {
-            let old_elem = self.values[index].clone();
+        if self.undo_log.in_snapshot() {
+            let old_elem = self.values.as_mut()[index].clone();
             self.undo_log.push(SetElem(index, old_elem));
         }
-        op(&mut self.values[index]);
+        op(&mut self.values.as_mut()[index]);
     }
+}
 
-    pub fn start_snapshot(&mut self) -> Snapshot {
-        self.num_open_snapshots += 1;
+impl<D, V, L> SnapshotVec<D, V, L>
+where
+    D: SnapshotVecDelegate,
+    V: VecLike<D> + Rollback<UndoLog<D>>,
+    L: Snapshots<UndoLog<D>>,
+{
+    pub fn start_snapshot(&mut self) -> Snapshot<L::Snapshot> {
         Snapshot {
             value_count: self.values.len(),
-            undo_len: self.undo_log.len(),
+            snapshot: self.undo_log.start_snapshot(),
         }
     }
 
-    pub fn actions_since_snapshot(&self, snapshot: &Snapshot) -> &[UndoLog<D>] {
-        &self.undo_log[snapshot.undo_len..]
-    }
-
-    fn assert_open_snapshot(&self, snapshot: &Snapshot) {
-        // Failures here may indicate a failure to follow a stack discipline.
-        assert!(self.undo_log.len() >= snapshot.undo_len);
-        assert!(self.num_open_snapshots > 0);
+    pub fn actions_since_snapshot(&self, snapshot: &Snapshot<L::Snapshot>) -> &[UndoLog<D>] {
+        self.undo_log.actions_since_snapshot(&snapshot.snapshot)
     }
 
-    pub fn rollback_to(&mut self, snapshot: Snapshot) {
-        debug!("rollback_to({})", snapshot.undo_len);
-
-        self.assert_open_snapshot(&snapshot);
-
-        while self.undo_log.len() > snapshot.undo_len {
-            match self.undo_log.pop().unwrap() {
-                NewElem(i) => {
-                    self.values.pop();
-                    assert!(self.values.len() == i);
-                }
-
-                SetElem(i, v) => {
-                    self.values[i] = v;
-                }
-
-                Other(u) => {
-                    D::reverse(&mut self.values, u);
-                }
-            }
-        }
-
-        self.num_open_snapshots -= 1;
+    pub fn rollback_to(&mut self, snapshot: Snapshot<L::Snapshot>) {
+        let values = &mut self.values;
+        self.undo_log.rollback_to(|| values, snapshot.snapshot);
     }
 
     /// Commits all changes since the last snapshot. Of course, they
     /// can still be undone if there is a snapshot further out.
-    pub fn commit(&mut self, snapshot: Snapshot) {
-        debug!("commit({})", snapshot.undo_len);
-
-        self.assert_open_snapshot(&snapshot);
-
-        if self.num_open_snapshots == 1 {
-            // The root snapshot. It's safe to clear the undo log because
-            // there's no snapshot further out that we might need to roll back
-            // to.
-            assert!(snapshot.undo_len == 0);
-            self.undo_log.clear();
-        }
-
-        self.num_open_snapshots -= 1;
+    pub fn commit(&mut self, snapshot: Snapshot<L::Snapshot>) {
+        self.undo_log.commit(snapshot.snapshot);
     }
 }
 
-impl<D: SnapshotVecDelegate> ops::Deref for SnapshotVec<D> {
+impl<D: SnapshotVecDelegate, V: VecLike<D>, L> ops::Deref for SnapshotVec<D, V, L> {
     type Target = [D::Value];
     fn deref(&self) -> &[D::Value] {
-        &*self.values
+        self.values.as_ref()
     }
 }
 
-impl<D: SnapshotVecDelegate> ops::DerefMut for SnapshotVec<D> {
+impl<D: SnapshotVecDelegate, V: VecLike<D>, L> ops::DerefMut for SnapshotVec<D, V, L> {
     fn deref_mut(&mut self) -> &mut [D::Value] {
-        &mut *self.values
+        self.values.as_mut()
     }
 }
 
-impl<D: SnapshotVecDelegate> ops::Index<usize> for SnapshotVec<D> {
+impl<D: SnapshotVecDelegate, V: VecLike<D>, L> ops::Index<usize> for SnapshotVec<D, V, L> {
     type Output = D::Value;
     fn index(&self, index: usize) -> &D::Value {
         self.get(index)
     }
 }
 
-impl<D: SnapshotVecDelegate> ops::IndexMut<usize> for SnapshotVec<D> {
+impl<D: SnapshotVecDelegate, V: VecLike<D>, L> ops::IndexMut<usize> for SnapshotVec<D, V, L> {
     fn index_mut(&mut self, index: usize) -> &mut D::Value {
         self.get_mut(index)
     }
 }
 
-impl<D: SnapshotVecDelegate> Extend<D::Value> for SnapshotVec<D> {
+impl<D: SnapshotVecDelegate, V: VecLike<D> + Extend<D::Value>> Extend<D::Value>
+    for SnapshotVec<D, V>
+{
     fn extend<T>(&mut self, iterable: T)
     where
         T: IntoIterator<Item = D::Value>,
@@ -272,21 +320,22 @@ impl<D: SnapshotVecDelegate> Extend<D::Value> for SnapshotVec<D> {
         let final_len = self.values.len();
 
         if self.in_snapshot() {
-            self.undo_log.extend((initial_len..final_len).map(|len| NewElem(len)));
+            self.undo_log
+                .extend((initial_len..final_len).map(|len| NewElem(len)));
         }
     }
 }
 
-impl<D: SnapshotVecDelegate> Clone for SnapshotVec<D>
+impl<D: SnapshotVecDelegate, V, L> Clone for SnapshotVec<D, V, L>
 where
-    D::Value: Clone,
-    D::Undo: Clone,
+    V: VecLike<D> + Clone,
+    L: Clone,
 {
     fn clone(&self) -> Self {
         SnapshotVec {
             values: self.values.clone(),
             undo_log: self.undo_log.clone(),
-            num_open_snapshots: self.num_open_snapshots,
+            _marker: PhantomData,
         }
     }
 }
diff --git a/src/undo_log.rs b/src/undo_log.rs
new file mode 100644
index 0000000..ba22cda
--- /dev/null
+++ b/src/undo_log.rs
@@ -0,0 +1,207 @@
+pub trait UndoLogs<T> {
+    fn in_snapshot(&self) -> bool {
+        self.num_open_snapshots() > 0
+    }
+    fn num_open_snapshots(&self) -> usize {
+        0
+    }
+    fn push(&mut self, undo: T);
+    fn clear(&mut self);
+    fn extend<I>(&mut self, undos: I)
+    where
+        Self: Sized,
+        I: IntoIterator<Item = T>,
+    {
+        undos.into_iter().for_each(|undo| self.push(undo));
+    }
+}
+
+impl<'a, T, U> UndoLogs<T> for &'a mut U
+where
+    U: UndoLogs<T>,
+{
+    fn in_snapshot(&self) -> bool {
+        (**self).in_snapshot()
+    }
+    fn num_open_snapshots(&self) -> usize {
+        (**self).num_open_snapshots()
+    }
+    fn push(&mut self, undo: T) {
+        (**self).push(undo)
+    }
+    fn clear(&mut self) {
+        (**self).clear();
+    }
+    fn extend<I>(&mut self, undos: I)
+    where
+        Self: Sized,
+        I: IntoIterator<Item = T>,
+    {
+        (**self).extend(undos)
+    }
+}
+
+pub trait Snapshots<T>: UndoLogs<T> {
+    type Snapshot;
+    fn has_changes(&self, snapshot: &Self::Snapshot) -> bool {
+        !self.actions_since_snapshot(snapshot).is_empty()
+    }
+    fn actions_since_snapshot(&self, snapshot: &Self::Snapshot) -> &[T];
+
+    fn start_snapshot(&mut self) -> Self::Snapshot;
+    fn rollback_to<R>(&mut self, values: impl FnOnce() -> R, snapshot: Self::Snapshot)
+    where
+        R: Rollback<T>;
+
+    fn commit(&mut self, snapshot: Self::Snapshot);
+}
+
+impl<T, U> Snapshots<T> for &'_ mut U
+where
+    U: Snapshots<T>,
+{
+    type Snapshot = U::Snapshot;
+    fn has_changes(&self, snapshot: &Self::Snapshot) -> bool {
+        (**self).has_changes(snapshot)
+    }
+    fn actions_since_snapshot(&self, snapshot: &Self::Snapshot) -> &[T] {
+        (**self).actions_since_snapshot(snapshot)
+    }
+
+    fn start_snapshot(&mut self) -> Self::Snapshot {
+        (**self).start_snapshot()
+    }
+    fn rollback_to<R>(&mut self, values: impl FnOnce() -> R, snapshot: Self::Snapshot)
+    where
+        R: Rollback<T>,
+    {
+        (**self).rollback_to(values, snapshot)
+    }
+
+    fn commit(&mut self, snapshot: Self::Snapshot) {
+        (**self).commit(snapshot)
+    }
+}
+
+pub struct NoUndo;
+impl<T> UndoLogs<T> for NoUndo {
+    fn num_open_snapshots(&self) -> usize {
+        0
+    }
+    fn push(&mut self, _undo: T) {}
+    fn clear(&mut self) {}
+}
+
+#[derive(Clone, Debug)]
+pub struct VecLog<T> {
+    log: Vec<T>,
+    num_open_snapshots: usize,
+}
+
+impl<T> Default for VecLog<T> {
+    fn default() -> Self {
+        VecLog {
+            log: Vec::new(),
+            num_open_snapshots: 0,
+        }
+    }
+}
+
+impl<T> UndoLogs<T> for VecLog<T> {
+    fn num_open_snapshots(&self) -> usize {
+        self.num_open_snapshots
+    }
+    fn push(&mut self, undo: T) {
+        self.log.push(undo);
+    }
+    fn clear(&mut self) {
+        self.log.clear();
+        self.num_open_snapshots = 0;
+    }
+}
+
+impl<T> Snapshots<T> for VecLog<T> {
+    type Snapshot = Snapshot;
+
+    fn has_changes(&self, snapshot: &Self::Snapshot) -> bool {
+        self.log.len() > snapshot.undo_len
+    }
+    fn actions_since_snapshot(&self, snapshot: &Snapshot) -> &[T] {
+        &self.log[snapshot.undo_len..]
+    }
+
+    fn start_snapshot(&mut self) -> Snapshot {
+        self.num_open_snapshots += 1;
+        Snapshot {
+            undo_len: self.log.len(),
+        }
+    }
+
+    fn rollback_to<R>(&mut self, values: impl FnOnce() -> R, snapshot: Snapshot)
+    where
+        R: Rollback<T>,
+    {
+        debug!("rollback_to({})", snapshot.undo_len);
+
+        self.assert_open_snapshot(&snapshot);
+
+        if self.log.len() > snapshot.undo_len {
+            let mut values = values();
+            while self.log.len() > snapshot.undo_len {
+                values.reverse(self.log.pop().unwrap());
+            }
+        }
+
+        self.num_open_snapshots -= 1;
+    }
+
+    fn commit(&mut self, snapshot: Snapshot) {
+        debug!("commit({})", snapshot.undo_len);
+
+        self.assert_open_snapshot(&snapshot);
+
+        if self.num_open_snapshots == 1 {
+            // The root snapshot. It's safe to clear the undo log because
+            // there's no snapshot further out that we might need to roll back
+            // to.
+            assert!(snapshot.undo_len == 0);
+            self.log.clear();
+        }
+
+        self.num_open_snapshots -= 1;
+    }
+}
+
+impl<T> VecLog<T> {
+    fn assert_open_snapshot(&self, snapshot: &Snapshot) {
+        // Failures here may indicate a failure to follow a stack discipline.
+        assert!(self.log.len() >= snapshot.undo_len);
+        assert!(self.num_open_snapshots > 0);
+    }
+}
+
+impl<T> std::ops::Index<usize> for VecLog<T> {
+    type Output = T;
+    fn index(&self, key: usize) -> &T {
+        &self.log[key]
+    }
+}
+
+pub trait Rollback<U> {
+    fn reverse(&mut self, undo: U);
+}
+
+impl<T, U> Rollback<U> for &'_ mut T
+where
+    T: Rollback<U>,
+{
+    fn reverse(&mut self, undo: U) {
+        (**self).reverse(undo)
+    }
+}
+
+// Snapshots are tokens that should be created/consumed linearly.
+pub struct Snapshot {
+    // Length of the undo log at the time the snapshot was taken.
+    undo_len: usize,
+}
diff --git a/src/unify/backing_vec.rs b/src/unify/backing_vec.rs
index ea4cca5..8a8f549 100644
--- a/src/unify/backing_vec.rs
+++ b/src/unify/backing_vec.rs
@@ -1,37 +1,25 @@
 #[cfg(feature = "persistent")]
 use dogged::DVec;
 use snapshot_vec as sv;
-use std::ops::{self, Range};
 use std::marker::PhantomData;
+use std::ops::{self, Range};
+
+use undo_log::{Snapshots, UndoLogs, VecLog};
 
-use super::{VarValue, UnifyKey, UnifyValue};
+use super::{UnifyKey, UnifyValue, VarValue};
 
 #[allow(dead_code)] // rustc BUG
 #[allow(type_alias_bounds)]
-type Key<S: UnificationStore> = <S as UnificationStore>::Key;
+type Key<S: UnificationStoreBase> = <S as UnificationStoreBase>::Key;
 
 /// Largely internal trait implemented by the unification table
 /// backing store types. The most common such type is `InPlace`,
 /// which indicates a standard, mutable unification table.
-pub trait UnificationStore:
-    ops::Index<usize, Output = VarValue<Key<Self>>> + Clone + Default
-{
+pub trait UnificationStoreBase: ops::Index<usize, Output = VarValue<Key<Self>>> {
     type Key: UnifyKey<Value = Self::Value>;
     type Value: UnifyValue;
-    type Snapshot;
 
-    fn start_snapshot(&mut self) -> Self::Snapshot;
-
-    fn rollback_to(&mut self, snapshot: Self::Snapshot);
-
-    fn commit(&mut self, snapshot: Self::Snapshot);
-
-    fn values_since_snapshot(&self, snapshot: &Self::Snapshot) -> Range<usize>;
-
-    fn reset_unifications(
-        &mut self,
-        value: impl FnMut(u32) -> VarValue<Self::Key>,
-    );
+    fn reset_unifications(&mut self, value: impl FnMut(u32) -> VarValue<Self::Key>);
 
     fn len(&self) -> usize;
 
@@ -40,84 +28,116 @@ pub trait UnificationStore:
     fn reserve(&mut self, num_new_values: usize);
 
     fn update<F>(&mut self, index: usize, op: F)
-        where F: FnOnce(&mut VarValue<Self::Key>);
+    where
+        F: FnOnce(&mut VarValue<Self::Key>);
 
     fn tag() -> &'static str {
         Self::Key::tag()
     }
 }
 
+pub trait UnificationStore: UnificationStoreBase {
+    type Snapshot;
+
+    fn start_snapshot(&mut self) -> Self::Snapshot;
+
+    fn rollback_to(&mut self, snapshot: Self::Snapshot);
+
+    fn commit(&mut self, snapshot: Self::Snapshot);
+
+    fn values_since_snapshot(&self, snapshot: &Self::Snapshot) -> Range<usize>;
+}
+
 /// Backing store for an in-place unification table.
 /// Not typically used directly.
 #[derive(Clone, Debug)]
-pub struct InPlace<K: UnifyKey> {
-    values: sv::SnapshotVec<Delegate<K>>
+pub struct InPlace<
+    K: UnifyKey,
+    V: sv::VecLike<Delegate<K>> = Vec<VarValue<K>>,
+    L = VecLog<sv::UndoLog<Delegate<K>>>,
+> {
+    pub(crate) values: sv::SnapshotVec<Delegate<K>, V, L>,
 }
 
 // HACK(eddyb) manual impl avoids `Default` bound on `K`.
-impl<K: UnifyKey> Default for InPlace<K> {
+impl<K: UnifyKey, V: sv::VecLike<Delegate<K>> + Default, L: Default> Default for InPlace<K, V, L> {
     fn default() -> Self {
-        InPlace { values: sv::SnapshotVec::new() }
+        InPlace {
+            values: sv::SnapshotVec::new(),
+        }
     }
 }
 
-impl<K: UnifyKey> UnificationStore for InPlace<K> {
+impl<K, V, L> UnificationStoreBase for InPlace<K, V, L>
+where
+    K: UnifyKey,
+    V: sv::VecLike<Delegate<K>>,
+    L: UndoLogs<sv::UndoLog<Delegate<K>>>,
+{
     type Key = K;
     type Value = K::Value;
-    type Snapshot = sv::Snapshot;
 
     #[inline]
-    fn start_snapshot(&mut self) -> Self::Snapshot {
-        self.values.start_snapshot()
+    fn reset_unifications(&mut self, mut value: impl FnMut(u32) -> VarValue<Self::Key>) {
+        self.values.set_all(|i| value(i as u32));
     }
 
-    #[inline]
-    fn rollback_to(&mut self, snapshot: Self::Snapshot) {
-        self.values.rollback_to(snapshot);
+    fn len(&self) -> usize {
+        self.values.len()
     }
 
     #[inline]
-    fn commit(&mut self, snapshot: Self::Snapshot) {
-        self.values.commit(snapshot);
+    fn push(&mut self, value: VarValue<Self::Key>) {
+        self.values.push(value);
     }
 
     #[inline]
-    fn values_since_snapshot(&self, snapshot: &Self::Snapshot) -> Range<usize> {
-        snapshot.value_count..self.len()
+    fn reserve(&mut self, num_new_values: usize) {
+        self.values.reserve(num_new_values);
     }
 
     #[inline]
-    fn reset_unifications(
-        &mut self,
-        mut value: impl FnMut(u32) -> VarValue<Self::Key>,
-    ) {
-        self.values.set_all(|i| value(i as u32));
+    fn update<F>(&mut self, index: usize, op: F)
+    where
+        F: FnOnce(&mut VarValue<Self::Key>),
+    {
+        self.values.update(index, op)
     }
+}
 
-    fn len(&self) -> usize {
-        self.values.len()
+impl<K, V, L> UnificationStore for InPlace<K, V, L>
+where
+    K: UnifyKey,
+    V: sv::VecLike<Delegate<K>>,
+    L: Snapshots<sv::UndoLog<Delegate<K>>>,
+{
+    type Snapshot = sv::Snapshot<L::Snapshot>;
+
+    #[inline]
+    fn start_snapshot(&mut self) -> Self::Snapshot {
+        self.values.start_snapshot()
     }
 
     #[inline]
-    fn push(&mut self, value: VarValue<Self::Key>) {
-        self.values.push(value);
+    fn rollback_to(&mut self, snapshot: Self::Snapshot) {
+        self.values.rollback_to(snapshot);
     }
 
     #[inline]
-    fn reserve(&mut self, num_new_values: usize) {
-        self.values.reserve(num_new_values);
+    fn commit(&mut self, snapshot: Self::Snapshot) {
+        self.values.commit(snapshot);
     }
 
     #[inline]
-    fn update<F>(&mut self, index: usize, op: F)
-        where F: FnOnce(&mut VarValue<Self::Key>)
-    {
-        self.values.update(index, op)
+    fn values_since_snapshot(&self, snapshot: &Self::Snapshot) -> Range<usize> {
+        snapshot.value_count..self.len()
     }
 }
 
-impl<K> ops::Index<usize> for InPlace<K>
-    where K: UnifyKey
+impl<K, V, L> ops::Index<usize> for InPlace<K, V, L>
+where
+    V: sv::VecLike<Delegate<K>>,
+    K: UnifyKey,
 {
     type Output = VarValue<K>;
     fn index(&self, index: usize) -> &VarValue<K> {
@@ -125,8 +145,9 @@ impl<K> ops::Index<usize> for InPlace<K>
     }
 }
 
+#[doc(hidden)]
 #[derive(Copy, Clone, Debug)]
-struct Delegate<K>(PhantomData<K>);
+pub struct Delegate<K>(PhantomData<K>);
 
 impl<K: UnifyKey> sv::SnapshotVecDelegate for Delegate<K> {
     type Value = VarValue<K>;
@@ -138,14 +159,16 @@ impl<K: UnifyKey> sv::SnapshotVecDelegate for Delegate<K> {
 #[cfg(feature = "persistent")]
 #[derive(Clone, Debug)]
 pub struct Persistent<K: UnifyKey> {
-    values: DVec<VarValue<K>>
+    values: DVec<VarValue<K>>,
 }
 
 // HACK(eddyb) manual impl avoids `Default` bound on `K`.
 #[cfg(feature = "persistent")]
 impl<K: UnifyKey> Default for Persistent<K> {
     fn default() -> Self {
-        Persistent { values: DVec::new() }
+        Persistent {
+            values: DVec::new(),
+        }
     }
 }
 
@@ -174,14 +197,11 @@ impl<K: UnifyKey> UnificationStore for Persistent<K> {
     }
 
     #[inline]
-    fn reset_unifications(
-        &mut self,
-        mut value: impl FnMut(u32) -> VarValue<Self::Key>,
-    ) {
+    fn reset_unifications(&mut self, mut value: impl FnMut(u32) -> VarValue<Self::Key>) {
         // Without extending dogged, there isn't obviously a more
         // efficient way to do this. But it's pretty dumb. Maybe
         // dogged needs a `map`.
-        for i in 0 .. self.values.len() {
+        for i in 0..self.values.len() {
             self.values[i] = value(i as u32);
         }
     }
@@ -202,7 +222,8 @@ impl<K: UnifyKey> UnificationStore for Persistent<K> {
 
     #[inline]
     fn update<F>(&mut self, index: usize, op: F)
-        where F: FnOnce(&mut VarValue<Self::Key>)
+    where
+        F: FnOnce(&mut VarValue<Self::Key>),
     {
         let p = &mut self.values[index];
         op(p);
@@ -211,7 +232,8 @@ impl<K: UnifyKey> UnificationStore for Persistent<K> {
 
 #[cfg(feature = "persistent")]
 impl<K> ops::Index<usize> for Persistent<K>
-    where K: UnifyKey
+where
+    K: UnifyKey,
 {
     type Output = VarValue<K>;
     fn index(&self, index: usize) -> &VarValue<K> {
diff --git a/src/unify/mod.rs b/src/unify/mod.rs
index 08f9b28..aa7162d 100644
--- a/src/unify/mod.rs
+++ b/src/unify/mod.rs
@@ -31,17 +31,19 @@
 //! The best way to see how it is used is to read the `tests.rs` file;
 //! search for e.g. `UnitKey`.
 
-use std::marker;
 use std::fmt::Debug;
+use std::marker;
 use std::ops::Range;
 
+use snapshot_vec::{self as sv, UndoLog};
+use undo_log::{UndoLogs, VecLog};
+
 mod backing_vec;
-pub use self::backing_vec::{InPlace, UnificationStore};
+pub use self::backing_vec::{Delegate, InPlace, UnificationStore, UnificationStoreBase};
 
 #[cfg(feature = "persistent")]
 pub use self::backing_vec::Persistent;
 
-
 #[cfg(test)]
 mod tests;
 
@@ -156,10 +158,11 @@ pub struct NoError {
 /// time of the algorithm under control. For more information, see
 /// <http://en.wikipedia.org/wiki/Disjoint-set_data_structure>.
 #[derive(PartialEq, Clone, Debug)]
-pub struct VarValue<K: UnifyKey> { // FIXME pub
-    parent: K, // if equal to self, this is a root
+pub struct VarValue<K: UnifyKey> {
+    // FIXME pub
+    parent: K,       // if equal to self, this is a root
     value: K::Value, // value assigned (only relevant to root)
-    rank: u32, // max depth (only relevant to root)
+    rank: u32,       // max depth (only relevant to root)
 }
 
 /// Table of unification keys and their values. You must define a key type K
@@ -176,14 +179,20 @@ pub struct VarValue<K: UnifyKey> { // FIXME pub
 ///   - This implies that ordinary operations are quite a bit slower though.
 ///   - Requires the `persistent` feature be selected in your Cargo.toml file.
 #[derive(Clone, Debug, Default)]
-pub struct UnificationTable<S: UnificationStore> {
+pub struct UnificationTable<S: UnificationStoreBase> {
     /// Indicates the current value of each key.
     values: S,
 }
 
+pub type UnificationStorage<K> = Vec<VarValue<K>>;
+
 /// A unification table that uses an "in-place" vector.
 #[allow(type_alias_bounds)]
-pub type InPlaceUnificationTable<K: UnifyKey> = UnificationTable<InPlace<K>>;
+pub type InPlaceUnificationTable<
+    K: UnifyKey,
+    V: sv::VecLike<Delegate<K>> = Vec<VarValue<K>>,
+    L = VecLog<UndoLog<Delegate<K>>>,
+> = UnificationTable<InPlace<K, V, L>>;
 
 /// A unification table that uses a "persistent" vector.
 #[cfg(feature = "persistent")]
@@ -232,17 +241,33 @@ impl<K: UnifyKey> VarValue<K> {
         }
     }
 }
+impl<K, V, L> UnificationTable<InPlace<K, V, L>>
+where
+    K: UnifyKey,
+    V: sv::VecLike<Delegate<K>>,
+    L: UndoLogs<sv::UndoLog<Delegate<K>>>,
+{
+    pub fn with_log(values: V, undo_log: L) -> Self {
+        Self {
+            values: InPlace {
+                values: sv::SnapshotVec::with_log(values, undo_log),
+            },
+        }
+    }
+}
 
 // We can't use V:LatticeValue, much as I would like to,
 // because frequently the pattern is that V=Option<U> for some
 // other type parameter U, and we have no way to say
 // Option<U>:LatticeValue.
 
-impl<S: UnificationStore> UnificationTable<S> {
+impl<S: UnificationStoreBase + Default> UnificationTable<S> {
     pub fn new() -> Self {
         Self::default()
     }
+}
 
+impl<S: UnificationStore> UnificationTable<S> {
     /// Starts a new snapshot. Each snapshot must be either
     /// rolled back or committed in a "LIFO" (stack) order.
     pub fn snapshot(&mut self) -> Snapshot<S> {
@@ -266,6 +291,15 @@ impl<S: UnificationStore> UnificationTable<S> {
         self.values.commit(snapshot.snapshot);
     }
 
+    /// Returns the keys of all variables created since the `snapshot`.
+    pub fn vars_since_snapshot(&self, snapshot: &Snapshot<S>) -> Range<S::Key> {
+        let range = self.values.values_since_snapshot(&snapshot.snapshot);
+        S::Key::from_index(range.start as u32)..S::Key::from_index(range.end as u32)
+    }
+}
+
+impl<S: UnificationStoreBase> UnificationTable<S> {
+    /// Starts a new snapshot. Each snapshot must be either
     /// Creates a fresh key with the given value.
     pub fn new_key(&mut self, value: S::Value) -> S::Key {
         let len = self.values.len();
@@ -284,10 +318,7 @@ impl<S: UnificationStore> UnificationTable<S> {
     /// Clears all unifications that have been performed, resetting to
     /// the initial state. The values of each variable are given by
     /// the closure.
-    pub fn reset_unifications(
-        &mut self,
-        mut value: impl FnMut(S::Key) -> S::Value,
-    ) {
+    pub fn reset_unifications(&mut self, mut value: impl FnMut(S::Key) -> S::Value) {
         self.values.reset_unifications(|i| {
             let key = UnifyKey::from_index(i as u32);
             let value = value(key);
@@ -300,15 +331,6 @@ impl<S: UnificationStore> UnificationTable<S> {
         self.values.len()
     }
 
-    /// Returns the keys of all variables created since the `snapshot`.
-    pub fn vars_since_snapshot(
-        &self,
-        snapshot: &Snapshot<S>,
-    ) -> Range<S::Key> {
-        let range = self.values.values_since_snapshot(&snapshot.snapshot);
-        S::Key::from_index(range.start as u32)..S::Key::from_index(range.end as u32)
-    }
-
     /// Obtains the current value for a particular key.
     /// Not for end-users; they can use `probe_value`.
     fn value(&self, key: S::Key) -> &VarValue<S::Key> {
@@ -370,13 +392,12 @@ impl<S: UnificationStore> UnificationTable<S> {
 
         let rank_a = self.value(key_a).rank;
         let rank_b = self.value(key_b).rank;
-        if let Some((new_root, redirected)) =
-            S::Key::order_roots(
-                key_a,
-                &self.value(key_a).value,
-                key_b,
-                &self.value(key_b).value,
-            ) {
+        if let Some((new_root, redirected)) = S::Key::order_roots(
+            key_a,
+            &self.value(key_a).value,
+            key_b,
+            &self.value(key_b).value,
+        ) {
             // compute the new rank for the new root that they chose;
             // this may not be the optimal choice.
             let new_rank = if new_root == key_a {
@@ -435,7 +456,7 @@ impl<S: UnificationStore> UnificationTable<S> {
 
 impl<S, K, V> UnificationTable<S>
 where
-    S: UnificationStore<Key = K, Value = V>,
+    S: UnificationStoreBase<Key = K, Value = V>,
     K: UnifyKey<Value = V>,
     V: UnifyValue,
 {
@@ -537,7 +558,6 @@ where
     }
 }
 
-
 ///////////////////////////////////////////////////////////////////////////
 
 impl UnifyValue for () {
@@ -554,14 +574,11 @@ impl<V: UnifyValue> UnifyValue for Option<V> {
     fn unify_values(a: &Option<V>, b: &Option<V>) -> Result<Self, V::Error> {
         match (a, b) {
             (&None, &None) => Ok(None),
-            (&Some(ref v), &None) |
-            (&None, &Some(ref v)) => Ok(Some(v.clone())),
-            (&Some(ref a), &Some(ref b)) => {
-                match V::unify_values(a, b) {
-                    Ok(v) => Ok(Some(v)),
-                    Err(err) => Err(err),
-                }
-            }
+            (&Some(ref v), &None) | (&None, &Some(ref v)) => Ok(Some(v.clone())),
+            (&Some(ref a), &Some(ref b)) => match V::unify_values(a, b) {
+                Ok(v) => Ok(Some(v)),
+                Err(err) => Err(err),
+            },
         }
     }
 }
diff --git a/src/unify/tests.rs b/src/unify/tests.rs
index 481fd44..74d04b0 100644
--- a/src/unify/tests.rs
+++ b/src/unify/tests.rs
@@ -17,10 +17,10 @@ extern crate test;
 #[cfg(feature = "bench")]
 use self::test::Bencher;
 use std::cmp;
-use unify::{NoError, InPlace, InPlaceUnificationTable, UnifyKey, EqUnifyValue, UnifyValue};
-use unify::{UnificationStore, UnificationTable};
 #[cfg(feature = "persistent")]
 use unify::Persistent;
+use unify::{EqUnifyValue, InPlace, InPlaceUnificationTable, NoError, UnifyKey, UnifyValue};
+use unify::{UnificationStore, UnificationTable};
 
 #[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)]
 struct UnitKey(u32);
@@ -40,7 +40,9 @@ impl UnifyKey for UnitKey {
 
 macro_rules! all_modes {
     ($name:ident for $t:ty => $body:tt) => {
-        fn test_body<$name: UnificationStore<Key = $t, Value = <$t as UnifyKey>::Value>>() {
+        fn test_body<
+            $name: Clone + Default + UnificationStore<Key = $t, Value = <$t as UnifyKey>::Value>,
+        >() {
             $body
         }
 
@@ -48,7 +50,7 @@ macro_rules! all_modes {
 
         #[cfg(feature = "persistent")]
         test_body::<Persistent<$t>>();
-    }
+    };
 }
 
 #[test]
@@ -91,7 +93,7 @@ fn big_array() {
 }
 
 #[cfg(feature = "bench")]
-fn big_array_bench_generic<S: UnificationStore<Key=UnitKey, Value=()>>(b: &mut Bencher) {
+fn big_array_bench_generic<S: UnificationStore<Key = UnitKey, Value = ()>>(b: &mut Bencher) {
     let mut ut: UnificationTable<S> = UnificationTable::new();
     let mut keys = Vec::new();
     const MAX: usize = 1 << 15;
@@ -126,7 +128,9 @@ fn big_array_bench_Persistent(b: &mut Bencher) {
 }
 
 #[cfg(feature = "bench")]
-fn big_array_bench_in_snapshot_generic<S: UnificationStore<Key=UnitKey, Value=()>>(b: &mut Bencher) {
+fn big_array_bench_in_snapshot_generic<S: UnificationStore<Key = UnitKey, Value = ()>>(
+    b: &mut Bencher,
+) {
     let mut ut: UnificationTable<S> = UnificationTable::new();
     let mut keys = Vec::new();
     const MAX: usize = 1 << 15;
@@ -165,7 +169,7 @@ fn big_array_bench_in_snapshot_Persistent(b: &mut Bencher) {
 }
 
 #[cfg(feature = "bench")]
-fn big_array_bench_clone_generic<S: UnificationStore<Key=UnitKey, Value=()>>(b: &mut Bencher) {
+fn big_array_bench_clone_generic<S: UnificationStore<Key = UnitKey, Value = ()>>(b: &mut Bencher) {
     let mut ut: UnificationTable<S> = UnificationTable::new();
     let mut keys = Vec::new();
     const MAX: usize = 1 << 15;

From f10d1d3b127a078d58315b8fed7c71c72725d28a Mon Sep 17 00:00:00 2001
From: Markus Westerlind <markus.westerlind@distilnetworks.com>
Date: Wed, 26 Feb 2020 08:04:42 +0100
Subject: [PATCH 02/18] Prevent 'Storage' types from being modified without a
 log

---
 src/snapshot_vec.rs | 12 ++++++++++--
 src/unify/mod.rs    | 18 ++++++++++++------
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/src/snapshot_vec.rs b/src/snapshot_vec.rs
index b667abe..9dec719 100644
--- a/src/snapshot_vec.rs
+++ b/src/snapshot_vec.rs
@@ -98,6 +98,10 @@ where
     }
 }
 
+#[allow(type_alias_bounds)]
+pub type SnapshotVecStorage<D: SnapshotVecDelegate> =
+    SnapshotVec<D, Vec<<D as SnapshotVecDelegate>::Value>, ()>;
+
 pub struct SnapshotVec<
     D: SnapshotVecDelegate,
     V: VecLike<D> = Vec<<D as SnapshotVecDelegate>::Value>,
@@ -153,9 +157,13 @@ impl<D: SnapshotVecDelegate, V: VecLike<D> + Default, L: Default> SnapshotVec<D,
 }
 
 impl<D: SnapshotVecDelegate, V: VecLike<D>, L> SnapshotVec<D, V, L> {
-    pub fn with_log(values: V, undo_log: L) -> Self {
+    pub fn with_log<'a, L2>(&'a mut self, undo_log: L2) -> SnapshotVec<D, &'a mut V, L2>
+    where
+        L2: UndoLogs<UndoLog<D>>,
+        &'a mut V: VecLike<D>,
+    {
         SnapshotVec {
-            values,
+            values: &mut self.values,
             undo_log,
             _marker: PhantomData,
         }
diff --git a/src/unify/mod.rs b/src/unify/mod.rs
index aa7162d..8ee9658 100644
--- a/src/unify/mod.rs
+++ b/src/unify/mod.rs
@@ -179,12 +179,12 @@ pub struct VarValue<K: UnifyKey> {
 ///   - This implies that ordinary operations are quite a bit slower though.
 ///   - Requires the `persistent` feature be selected in your Cargo.toml file.
 #[derive(Clone, Debug, Default)]
-pub struct UnificationTable<S: UnificationStoreBase> {
+pub struct UnificationTable<S> {
     /// Indicates the current value of each key.
     values: S,
 }
 
-pub type UnificationStorage<K> = Vec<VarValue<K>>;
+pub type UnificationTableStorage<K> = UnificationTable<InPlace<K, Vec<VarValue<K>>, ()>>;
 
 /// A unification table that uses an "in-place" vector.
 #[allow(type_alias_bounds)]
@@ -245,12 +245,18 @@ impl<K, V, L> UnificationTable<InPlace<K, V, L>>
 where
     K: UnifyKey,
     V: sv::VecLike<Delegate<K>>,
-    L: UndoLogs<sv::UndoLog<Delegate<K>>>,
 {
-    pub fn with_log(values: V, undo_log: L) -> Self {
-        Self {
+    pub fn with_log<'a, L2>(
+        &'a mut self,
+        undo_log: L2,
+    ) -> UnificationTable<InPlace<K, &'a mut V, L2>>
+    where
+        L2: UndoLogs<sv::UndoLog<Delegate<K>>>,
+        &'a mut V: sv::VecLike<Delegate<K>>,
+    {
+        UnificationTable {
             values: InPlace {
-                values: sv::SnapshotVec::with_log(values, undo_log),
+                values: self.values.values.with_log(undo_log),
             },
         }
     }

From bd84def590ade05160cc4eac419ee57613070718 Mon Sep 17 00:00:00 2001
From: Markus Westerlind <markus.westerlind@distilnetworks.com>
Date: Wed, 26 Feb 2020 08:37:41 +0100
Subject: [PATCH 03/18] Add UNificationStorage back

---
 src/unify/mod.rs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/unify/mod.rs b/src/unify/mod.rs
index 8ee9658..62e6ff5 100644
--- a/src/unify/mod.rs
+++ b/src/unify/mod.rs
@@ -184,7 +184,8 @@ pub struct UnificationTable<S> {
     values: S,
 }
 
-pub type UnificationTableStorage<K> = UnificationTable<InPlace<K, Vec<VarValue<K>>, ()>>;
+pub type UnificationStorage<K> = Vec<VarValue<K>>;
+pub type UnificationTableStorage<K> = UnificationTable<InPlace<K, UnificationStorage<K>, ()>>;
 
 /// A unification table that uses an "in-place" vector.
 #[allow(type_alias_bounds)]

From 4b08d1c69db5c5d553ae72a9d24c53fcf431628c Mon Sep 17 00:00:00 2001
From: Markus Westerlind <markus.westerlind@distilnetworks.com>
Date: Wed, 26 Feb 2020 08:43:03 +0100
Subject: [PATCH 04/18] Allow Rollback on the wrapper storage types

---
 src/snapshot_vec.rs      | 5 +++++
 src/unify/backing_vec.rs | 8 +++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/snapshot_vec.rs b/src/snapshot_vec.rs
index 9dec719..cc93f38 100644
--- a/src/snapshot_vec.rs
+++ b/src/snapshot_vec.rs
@@ -40,6 +40,11 @@ pub enum UndoLog<D: SnapshotVecDelegate> {
     Other(D::Undo),
 }
 
+impl<D: SnapshotVecDelegate> Rollback<UndoLog<D>> for SnapshotVecStorage<D> {
+    fn reverse(&mut self, undo: UndoLog<D>) {
+        self.values.reverse(undo)
+    }
+}
 impl<D: SnapshotVecDelegate> Rollback<UndoLog<D>> for Vec<D::Value> {
     fn reverse(&mut self, undo: UndoLog<D>) {
         match undo {
diff --git a/src/unify/backing_vec.rs b/src/unify/backing_vec.rs
index 8a8f549..31cf435 100644
--- a/src/unify/backing_vec.rs
+++ b/src/unify/backing_vec.rs
@@ -4,7 +4,7 @@ use snapshot_vec as sv;
 use std::marker::PhantomData;
 use std::ops::{self, Range};
 
-use undo_log::{Snapshots, UndoLogs, VecLog};
+use undo_log::{Rollback, Snapshots, UndoLogs, VecLog};
 
 use super::{UnifyKey, UnifyValue, VarValue};
 
@@ -156,6 +156,12 @@ impl<K: UnifyKey> sv::SnapshotVecDelegate for Delegate<K> {
     fn reverse(_: &mut Vec<VarValue<K>>, _: ()) {}
 }
 
+impl<K: UnifyKey> Rollback<sv::UndoLog<Delegate<K>>> for super::UnificationTableStorage<K> {
+    fn reverse(&mut self, undo: sv::UndoLog<Delegate<K>>) {
+        self.values.values.reverse(undo);
+    }
+}
+
 #[cfg(feature = "persistent")]
 #[derive(Clone, Debug)]
 pub struct Persistent<K: UnifyKey> {

From 1f370f21f4c8f5548f53411535157e01e9139f1f Mon Sep 17 00:00:00 2001
From: Markus Westerlind <markus.westerlind@distilnetworks.com>
Date: Wed, 26 Feb 2020 08:47:58 +0100
Subject: [PATCH 05/18] Relax UnificationTable::new

---
 src/unify/mod.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/unify/mod.rs b/src/unify/mod.rs
index 62e6ff5..cc70d22 100644
--- a/src/unify/mod.rs
+++ b/src/unify/mod.rs
@@ -268,7 +268,7 @@ where
 // other type parameter U, and we have no way to say
 // Option<U>:LatticeValue.
 
-impl<S: UnificationStoreBase + Default> UnificationTable<S> {
+impl<S: Default> UnificationTable<S> {
     pub fn new() -> Self {
         Self::default()
     }

From 528af9af5e2c4f882c54c13bd81712135c05d274 Mon Sep 17 00:00:00 2001
From: Markus Westerlind <markus.westerlind@distilnetworks.com>
Date: Wed, 26 Feb 2020 09:44:26 +0100
Subject: [PATCH 06/18] Don't require UndoLogs for calling len

---
 src/unify/backing_vec.rs | 49 ++++++++++++++++++++--------------------
 src/unify/mod.rs         | 14 +++++++-----
 2 files changed, 32 insertions(+), 31 deletions(-)

diff --git a/src/unify/backing_vec.rs b/src/unify/backing_vec.rs
index 31cf435..7b02fee 100644
--- a/src/unify/backing_vec.rs
+++ b/src/unify/backing_vec.rs
@@ -4,7 +4,7 @@ use snapshot_vec as sv;
 use std::marker::PhantomData;
 use std::ops::{self, Range};
 
-use undo_log::{Rollback, Snapshots, UndoLogs, VecLog};
+use undo_log::{Rollback, Snapshots, VecLog};
 
 use super::{UnifyKey, UnifyValue, VarValue};
 
@@ -19,18 +19,8 @@ pub trait UnificationStoreBase: ops::Index<usize, Output = VarValue<Key<Self>>>
     type Key: UnifyKey<Value = Self::Value>;
     type Value: UnifyValue;
 
-    fn reset_unifications(&mut self, value: impl FnMut(u32) -> VarValue<Self::Key>);
-
     fn len(&self) -> usize;
 
-    fn push(&mut self, value: VarValue<Self::Key>);
-
-    fn reserve(&mut self, num_new_values: usize);
-
-    fn update<F>(&mut self, index: usize, op: F)
-    where
-        F: FnOnce(&mut VarValue<Self::Key>);
-
     fn tag() -> &'static str {
         Self::Key::tag()
     }
@@ -39,6 +29,16 @@ pub trait UnificationStoreBase: ops::Index<usize, Output = VarValue<Key<Self>>>
 pub trait UnificationStore: UnificationStoreBase {
     type Snapshot;
 
+    fn reset_unifications(&mut self, value: impl FnMut(u32) -> VarValue<Self::Key>);
+
+    fn push(&mut self, value: VarValue<Self::Key>);
+
+    fn reserve(&mut self, num_new_values: usize);
+
+    fn update<F>(&mut self, index: usize, op: F)
+    where
+        F: FnOnce(&mut VarValue<Self::Key>);
+
     fn start_snapshot(&mut self) -> Self::Snapshot;
 
     fn rollback_to(&mut self, snapshot: Self::Snapshot);
@@ -72,20 +72,28 @@ impl<K, V, L> UnificationStoreBase for InPlace<K, V, L>
 where
     K: UnifyKey,
     V: sv::VecLike<Delegate<K>>,
-    L: UndoLogs<sv::UndoLog<Delegate<K>>>,
 {
     type Key = K;
     type Value = K::Value;
 
+    fn len(&self) -> usize {
+        self.values.len()
+    }
+}
+
+impl<K, V, L> UnificationStore for InPlace<K, V, L>
+where
+    K: UnifyKey,
+    V: sv::VecLike<Delegate<K>>,
+    L: Snapshots<sv::UndoLog<Delegate<K>>>,
+{
+    type Snapshot = sv::Snapshot<L::Snapshot>;
+
     #[inline]
     fn reset_unifications(&mut self, mut value: impl FnMut(u32) -> VarValue<Self::Key>) {
         self.values.set_all(|i| value(i as u32));
     }
 
-    fn len(&self) -> usize {
-        self.values.len()
-    }
-
     #[inline]
     fn push(&mut self, value: VarValue<Self::Key>) {
         self.values.push(value);
@@ -103,15 +111,6 @@ where
     {
         self.values.update(index, op)
     }
-}
-
-impl<K, V, L> UnificationStore for InPlace<K, V, L>
-where
-    K: UnifyKey,
-    V: sv::VecLike<Delegate<K>>,
-    L: Snapshots<sv::UndoLog<Delegate<K>>>,
-{
-    type Snapshot = sv::Snapshot<L::Snapshot>;
 
     #[inline]
     fn start_snapshot(&mut self) -> Self::Snapshot {
diff --git a/src/unify/mod.rs b/src/unify/mod.rs
index cc70d22..1cf2fe6 100644
--- a/src/unify/mod.rs
+++ b/src/unify/mod.rs
@@ -306,6 +306,13 @@ impl<S: UnificationStore> UnificationTable<S> {
 }
 
 impl<S: UnificationStoreBase> UnificationTable<S> {
+    /// Returns the number of keys created so far.
+    pub fn len(&self) -> usize {
+        self.values.len()
+    }
+}
+
+impl<S: UnificationStore> UnificationTable<S> {
     /// Starts a new snapshot. Each snapshot must be either
     /// Creates a fresh key with the given value.
     pub fn new_key(&mut self, value: S::Value) -> S::Key {
@@ -333,11 +340,6 @@ impl<S: UnificationStoreBase> UnificationTable<S> {
         });
     }
 
-    /// Returns the number of keys created so far.
-    pub fn len(&self) -> usize {
-        self.values.len()
-    }
-
     /// Obtains the current value for a particular key.
     /// Not for end-users; they can use `probe_value`.
     fn value(&self, key: S::Key) -> &VarValue<S::Key> {
@@ -463,7 +465,7 @@ impl<S: UnificationStoreBase> UnificationTable<S> {
 
 impl<S, K, V> UnificationTable<S>
 where
-    S: UnificationStoreBase<Key = K, Value = V>,
+    S: UnificationStore<Key = K, Value = V>,
     K: UnifyKey<Value = V>,
     V: UnifyValue,
 {

From 25f94c22e885301e074b7dcbda90ba9fa4b28af1 Mon Sep 17 00:00:00 2001
From: Markus Westerlind <markus.westerlind@distilnetworks.com>
Date: Wed, 26 Feb 2020 09:46:07 +0100
Subject: [PATCH 07/18] Fix persistent tables

---
 src/unify/backing_vec.rs | 14 +++++++++-----
 src/unify/tests.rs       | 12 +++++++++---
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/src/unify/backing_vec.rs b/src/unify/backing_vec.rs
index 7b02fee..7f7b86d 100644
--- a/src/unify/backing_vec.rs
+++ b/src/unify/backing_vec.rs
@@ -178,9 +178,17 @@ impl<K: UnifyKey> Default for Persistent<K> {
 }
 
 #[cfg(feature = "persistent")]
-impl<K: UnifyKey> UnificationStore for Persistent<K> {
+impl<K: UnifyKey> UnificationStoreBase for Persistent<K> {
     type Key = K;
     type Value = K::Value;
+
+    fn len(&self) -> usize {
+        self.values.len()
+    }
+}
+
+#[cfg(feature = "persistent")]
+impl<K: UnifyKey> UnificationStore for Persistent<K> {
     type Snapshot = Self;
 
     #[inline]
@@ -211,10 +219,6 @@ impl<K: UnifyKey> UnificationStore for Persistent<K> {
         }
     }
 
-    fn len(&self) -> usize {
-        self.values.len()
-    }
-
     #[inline]
     fn push(&mut self, value: VarValue<Self::Key>) {
         self.values.push(value);
diff --git a/src/unify/tests.rs b/src/unify/tests.rs
index 74d04b0..5665aba 100644
--- a/src/unify/tests.rs
+++ b/src/unify/tests.rs
@@ -93,7 +93,9 @@ fn big_array() {
 }
 
 #[cfg(feature = "bench")]
-fn big_array_bench_generic<S: UnificationStore<Key = UnitKey, Value = ()>>(b: &mut Bencher) {
+fn big_array_bench_generic<S: Default + UnificationStore<Key = UnitKey, Value = ()>>(
+    b: &mut Bencher,
+) {
     let mut ut: UnificationTable<S> = UnificationTable::new();
     let mut keys = Vec::new();
     const MAX: usize = 1 << 15;
@@ -128,7 +130,7 @@ fn big_array_bench_Persistent(b: &mut Bencher) {
 }
 
 #[cfg(feature = "bench")]
-fn big_array_bench_in_snapshot_generic<S: UnificationStore<Key = UnitKey, Value = ()>>(
+fn big_array_bench_in_snapshot_generic<S: Default + UnificationStore<Key = UnitKey, Value = ()>>(
     b: &mut Bencher,
 ) {
     let mut ut: UnificationTable<S> = UnificationTable::new();
@@ -169,7 +171,11 @@ fn big_array_bench_in_snapshot_Persistent(b: &mut Bencher) {
 }
 
 #[cfg(feature = "bench")]
-fn big_array_bench_clone_generic<S: UnificationStore<Key = UnitKey, Value = ()>>(b: &mut Bencher) {
+fn big_array_bench_clone_generic<
+    S: Default + Clone + UnificationStore<Key = UnitKey, Value = ()>,
+>(
+    b: &mut Bencher,
+) {
     let mut ut: UnificationTable<S> = UnificationTable::new();
     let mut keys = Vec::new();
     const MAX: usize = 1 << 15;

From 30daa1d435c04bf23b67b9f28d10d93f3557fb50 Mon Sep 17 00:00:00 2001
From: Markus Westerlind <markus.westerlind@distilnetworks.com>
Date: Wed, 26 Feb 2020 09:52:44 +0100
Subject: [PATCH 08/18] Split UnificationStore into a Mut variant which do not
 require snapshotting

---
 src/unify/backing_vec.rs | 70 +++++++++++++++++++++++-----------------
 src/unify/mod.rs         |  8 +++--
 2 files changed, 46 insertions(+), 32 deletions(-)

diff --git a/src/unify/backing_vec.rs b/src/unify/backing_vec.rs
index 7f7b86d..7c1eb2c 100644
--- a/src/unify/backing_vec.rs
+++ b/src/unify/backing_vec.rs
@@ -4,7 +4,7 @@ use snapshot_vec as sv;
 use std::marker::PhantomData;
 use std::ops::{self, Range};
 
-use undo_log::{Rollback, Snapshots, VecLog};
+use undo_log::{Rollback, Snapshots, UndoLogs, VecLog};
 
 use super::{UnifyKey, UnifyValue, VarValue};
 
@@ -26,9 +26,7 @@ pub trait UnificationStoreBase: ops::Index<usize, Output = VarValue<Key<Self>>>
     }
 }
 
-pub trait UnificationStore: UnificationStoreBase {
-    type Snapshot;
-
+pub trait UnificationStoreMut: UnificationStoreBase {
     fn reset_unifications(&mut self, value: impl FnMut(u32) -> VarValue<Self::Key>);
 
     fn push(&mut self, value: VarValue<Self::Key>);
@@ -38,6 +36,10 @@ pub trait UnificationStore: UnificationStoreBase {
     fn update<F>(&mut self, index: usize, op: F)
     where
         F: FnOnce(&mut VarValue<Self::Key>);
+}
+
+pub trait UnificationStore: UnificationStoreMut {
+    type Snapshot;
 
     fn start_snapshot(&mut self) -> Self::Snapshot;
 
@@ -81,14 +83,12 @@ where
     }
 }
 
-impl<K, V, L> UnificationStore for InPlace<K, V, L>
+impl<K, V, L> UnificationStoreMut for InPlace<K, V, L>
 where
     K: UnifyKey,
     V: sv::VecLike<Delegate<K>>,
-    L: Snapshots<sv::UndoLog<Delegate<K>>>,
+    L: UndoLogs<sv::UndoLog<Delegate<K>>>,
 {
-    type Snapshot = sv::Snapshot<L::Snapshot>;
-
     #[inline]
     fn reset_unifications(&mut self, mut value: impl FnMut(u32) -> VarValue<Self::Key>) {
         self.values.set_all(|i| value(i as u32));
@@ -111,6 +111,15 @@ where
     {
         self.values.update(index, op)
     }
+}
+
+impl<K, V, L> UnificationStore for InPlace<K, V, L>
+where
+    K: UnifyKey,
+    V: sv::VecLike<Delegate<K>>,
+    L: Snapshots<sv::UndoLog<Delegate<K>>>,
+{
+    type Snapshot = sv::Snapshot<L::Snapshot>;
 
     #[inline]
     fn start_snapshot(&mut self) -> Self::Snapshot {
@@ -188,27 +197,7 @@ impl<K: UnifyKey> UnificationStoreBase for Persistent<K> {
 }
 
 #[cfg(feature = "persistent")]
-impl<K: UnifyKey> UnificationStore for Persistent<K> {
-    type Snapshot = Self;
-
-    #[inline]
-    fn start_snapshot(&mut self) -> Self::Snapshot {
-        self.clone()
-    }
-
-    #[inline]
-    fn rollback_to(&mut self, snapshot: Self::Snapshot) {
-        *self = snapshot;
-    }
-
-    #[inline]
-    fn commit(&mut self, _snapshot: Self::Snapshot) {}
-
-    #[inline]
-    fn values_since_snapshot(&self, snapshot: &Self::Snapshot) -> Range<usize> {
-        snapshot.len()..self.len()
-    }
-
+impl<K: UnifyKey> UnificationStoreMut for Persistent<K> {
     #[inline]
     fn reset_unifications(&mut self, mut value: impl FnMut(u32) -> VarValue<Self::Key>) {
         // Without extending dogged, there isn't obviously a more
@@ -239,6 +228,29 @@ impl<K: UnifyKey> UnificationStore for Persistent<K> {
     }
 }
 
+#[cfg(feature = "persistent")]
+impl<K: UnifyKey> UnificationStore for Persistent<K> {
+    type Snapshot = Self;
+
+    #[inline]
+    fn start_snapshot(&mut self) -> Self::Snapshot {
+        self.clone()
+    }
+
+    #[inline]
+    fn rollback_to(&mut self, snapshot: Self::Snapshot) {
+        *self = snapshot;
+    }
+
+    #[inline]
+    fn commit(&mut self, _snapshot: Self::Snapshot) {}
+
+    #[inline]
+    fn values_since_snapshot(&self, snapshot: &Self::Snapshot) -> Range<usize> {
+        snapshot.len()..self.len()
+    }
+}
+
 #[cfg(feature = "persistent")]
 impl<K> ops::Index<usize> for Persistent<K>
 where
diff --git a/src/unify/mod.rs b/src/unify/mod.rs
index 1cf2fe6..185e604 100644
--- a/src/unify/mod.rs
+++ b/src/unify/mod.rs
@@ -39,7 +39,9 @@ use snapshot_vec::{self as sv, UndoLog};
 use undo_log::{UndoLogs, VecLog};
 
 mod backing_vec;
-pub use self::backing_vec::{Delegate, InPlace, UnificationStore, UnificationStoreBase};
+pub use self::backing_vec::{
+    Delegate, InPlace, UnificationStore, UnificationStoreBase, UnificationStoreMut,
+};
 
 #[cfg(feature = "persistent")]
 pub use self::backing_vec::Persistent;
@@ -312,7 +314,7 @@ impl<S: UnificationStoreBase> UnificationTable<S> {
     }
 }
 
-impl<S: UnificationStore> UnificationTable<S> {
+impl<S: UnificationStoreMut> UnificationTable<S> {
     /// Starts a new snapshot. Each snapshot must be either
     /// Creates a fresh key with the given value.
     pub fn new_key(&mut self, value: S::Value) -> S::Key {
@@ -465,7 +467,7 @@ impl<S: UnificationStore> UnificationTable<S> {
 
 impl<S, K, V> UnificationTable<S>
 where
-    S: UnificationStore<Key = K, Value = V>,
+    S: UnificationStoreMut<Key = K, Value = V>,
     K: UnifyKey<Value = V>,
     V: UnifyValue,
 {

From ffbc9c315e63adb01c15f3f60fceafab922f508c Mon Sep 17 00:00:00 2001
From: Markus Westerlind <markus.westerlind@distilnetworks.com>
Date: Tue, 3 Mar 2020 10:17:54 +0100
Subject: [PATCH 09/18] Add some documentation

---
 src/snapshot_vec.rs |  3 +++
 src/undo_log.rs     | 10 +++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/snapshot_vec.rs b/src/snapshot_vec.rs
index cc93f38..235d629 100644
--- a/src/snapshot_vec.rs
+++ b/src/snapshot_vec.rs
@@ -156,12 +156,15 @@ impl<D: SnapshotVecDelegate, V: VecLike<D> + Default, L: Default> Default for Sn
 }
 
 impl<D: SnapshotVecDelegate, V: VecLike<D> + Default, L: Default> SnapshotVec<D, V, L> {
+    /// Creates a new `SnapshotVec`. If `L` is set to `()` then most mutating functions will not
+    /// be accessible without calling `with_log` and supplying a compatibly `UndoLogs` instance.
     pub fn new() -> Self {
         Self::default()
     }
 }
 
 impl<D: SnapshotVecDelegate, V: VecLike<D>, L> SnapshotVec<D, V, L> {
+    /// Creates a `SnapshotVec` using the `undo_log`, allowing mutating methods to be called
     pub fn with_log<'a, L2>(&'a mut self, undo_log: L2) -> SnapshotVec<D, &'a mut V, L2>
     where
         L2: UndoLogs<UndoLog<D>>,
diff --git a/src/undo_log.rs b/src/undo_log.rs
index ba22cda..ce8ca0c 100644
--- a/src/undo_log.rs
+++ b/src/undo_log.rs
@@ -1,3 +1,5 @@
+/// A trait which allows actions (`T`) to be pushed which which allows the action to be undone at a
+/// later time if needed
 pub trait UndoLogs<T> {
     fn in_snapshot(&self) -> bool {
         self.num_open_snapshots() > 0
@@ -41,6 +43,10 @@ where
     }
 }
 
+/// A trait which allows snapshots to be done at specific points. Each snapshot can then be used to
+/// rollback any changes to an underlying data structures if they were not desirable.
+///
+/// Each snapshot must be consumed linearly with either `rollback_to` or `commit`.
 pub trait Snapshots<T>: UndoLogs<T> {
     type Snapshot;
     fn has_changes(&self, snapshot: &Self::Snapshot) -> bool {
@@ -92,6 +98,7 @@ impl<T> UndoLogs<T> for NoUndo {
     fn clear(&mut self) {}
 }
 
+/// A basic undo log.
 #[derive(Clone, Debug)]
 pub struct VecLog<T> {
     log: Vec<T>,
@@ -187,6 +194,7 @@ impl<T> std::ops::Index<usize> for VecLog<T> {
     }
 }
 
+/// A trait implemented for types which can be rolled back using actions of type `U`.
 pub trait Rollback<U> {
     fn reverse(&mut self, undo: U);
 }
@@ -200,7 +208,7 @@ where
     }
 }
 
-// Snapshots are tokens that should be created/consumed linearly.
+/// Snapshots are tokens that should be created/consumed linearly.
 pub struct Snapshot {
     // Length of the undo log at the time the snapshot was taken.
     undo_len: usize,

From 5ae2cfa1d733c7a595f92b989eb6adf5ccaed7b6 Mon Sep 17 00:00:00 2001
From: Markus Westerlind <markus.westerlind@distilnetworks.com>
Date: Fri, 20 Mar 2020 10:43:07 +0100
Subject: [PATCH 10/18] docL Document the undo_log module

---
 src/undo_log.rs  | 10 ++++++++++
 src/unify/mod.rs |  1 -
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/undo_log.rs b/src/undo_log.rs
index ce8ca0c..4d79e39 100644
--- a/src/undo_log.rs
+++ b/src/undo_log.rs
@@ -1,3 +1,13 @@
+//! Module which contains the snapshot/rollback functionality of the `ena` data structures.
+//!
+//! For most usecases this is just an internal implementation detail. However if many `ena`
+//! data structures are used snapshotted simultaneously it is possible to use
+//! `UnificationTableStorage`/`SnapshotVecStorage` instead and use a custom `UndoLogs<T>`
+//! type capable of recording the actions of all used data structures.
+//!
+//! Since the `*Storage` variants do not have an undo log `with_log` must be called with the
+//! unified log before any mutating actions.
+
 /// A trait which allows actions (`T`) to be pushed which which allows the action to be undone at a
 /// later time if needed
 pub trait UndoLogs<T> {
diff --git a/src/unify/mod.rs b/src/unify/mod.rs
index 185e604..10a1bdb 100644
--- a/src/unify/mod.rs
+++ b/src/unify/mod.rs
@@ -161,7 +161,6 @@ pub struct NoError {
 /// <http://en.wikipedia.org/wiki/Disjoint-set_data_structure>.
 #[derive(PartialEq, Clone, Debug)]
 pub struct VarValue<K: UnifyKey> {
-    // FIXME pub
     parent: K,       // if equal to self, this is a root
     value: K::Value, // value assigned (only relevant to root)
     rank: u32,       // max depth (only relevant to root)

From fac8c2de85ab31d681556a0425fa2ee88307aa38 Mon Sep 17 00:00:00 2001
From: Markus Westerlind <markus.westerlind@distilnetworks.com>
Date: Fri, 20 Mar 2020 11:21:35 +0100
Subject: [PATCH 11/18] test: Check the unified undo log addition

---
 src/unify/tests.rs | 180 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 179 insertions(+), 1 deletion(-)

diff --git a/src/unify/tests.rs b/src/unify/tests.rs
index 5665aba..7b81b39 100644
--- a/src/unify/tests.rs
+++ b/src/unify/tests.rs
@@ -16,10 +16,14 @@
 extern crate test;
 #[cfg(feature = "bench")]
 use self::test::Bencher;
+use snapshot_vec as sv;
 use std::cmp;
+use undo_log::{Rollback, Snapshots, UndoLogs};
 #[cfg(feature = "persistent")]
 use unify::Persistent;
-use unify::{EqUnifyValue, InPlace, InPlaceUnificationTable, NoError, UnifyKey, UnifyValue};
+use unify::{
+    self as ut, EqUnifyValue, InPlace, InPlaceUnificationTable, NoError, UnifyKey, UnifyValue,
+};
 use unify::{UnificationStore, UnificationTable};
 
 #[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)]
@@ -484,3 +488,177 @@ fn clone_table() {
         }
     }
 }
+
+enum UndoLog {
+    EqRelation(sv::UndoLog<ut::Delegate<IntKey>>),
+    Values(sv::UndoLog<i32>),
+}
+
+impl From<sv::UndoLog<ut::Delegate<IntKey>>> for UndoLog {
+    fn from(l: sv::UndoLog<ut::Delegate<IntKey>>) -> Self {
+        UndoLog::EqRelation(l)
+    }
+}
+
+impl From<sv::UndoLog<i32>> for UndoLog {
+    fn from(l: sv::UndoLog<i32>) -> Self {
+        UndoLog::Values(l)
+    }
+}
+
+impl Rollback<UndoLog> for TypeVariableStorage {
+    fn reverse(&mut self, undo: UndoLog) {
+        match undo {
+            UndoLog::EqRelation(undo) => self.eq_relations.reverse(undo),
+            UndoLog::Values(undo) => self.values.reverse(undo),
+        }
+    }
+}
+
+#[derive(Default)]
+struct TypeVariableStorage {
+    values: sv::SnapshotVecStorage<i32>,
+
+    eq_relations: ut::UnificationTableStorage<IntKey>,
+}
+
+impl TypeVariableStorage {
+    fn with_log<'a>(&'a mut self, undo_log: &'a mut TypeVariableUndoLogs) -> TypeVariableTable<'a> {
+        TypeVariableTable {
+            storage: self,
+            undo_log,
+        }
+    }
+
+    fn len(&mut self) -> usize {
+        assert_eq!(self.values.len(), self.eq_relations.len());
+        self.values.len()
+    }
+}
+
+struct TypeVariableTable<'a> {
+    storage: &'a mut TypeVariableStorage,
+
+    undo_log: &'a mut TypeVariableUndoLogs,
+}
+
+impl TypeVariableTable<'_> {
+    fn new(&mut self, i: i32) -> IntKey {
+        self.storage.values.with_log(&mut self.undo_log).push(i);
+        self.storage
+            .eq_relations
+            .with_log(&mut self.undo_log)
+            .new_key(None)
+    }
+}
+
+struct Snapshot {
+    undo_len: usize,
+}
+
+struct TypeVariableUndoLogs {
+    logs: Vec<UndoLog>,
+    num_open_snapshots: usize,
+}
+
+impl Default for TypeVariableUndoLogs {
+    fn default() -> Self {
+        Self {
+            logs: Default::default(),
+            num_open_snapshots: Default::default(),
+        }
+    }
+}
+
+impl<T> UndoLogs<T> for TypeVariableUndoLogs
+where
+    UndoLog: From<T>,
+{
+    fn num_open_snapshots(&self) -> usize {
+        self.num_open_snapshots
+    }
+    fn push(&mut self, undo: T) {
+        if self.in_snapshot() {
+            self.logs.push(undo.into())
+        }
+    }
+    fn clear(&mut self) {
+        self.logs.clear();
+        self.num_open_snapshots = 0;
+    }
+    fn extend<J>(&mut self, undos: J)
+    where
+        Self: Sized,
+        J: IntoIterator<Item = T>,
+    {
+        if self.in_snapshot() {
+            self.logs.extend(undos.into_iter().map(UndoLog::from))
+        }
+    }
+}
+
+impl Snapshots<UndoLog> for TypeVariableUndoLogs {
+    type Snapshot = Snapshot;
+    fn actions_since_snapshot(&self, snapshot: &Self::Snapshot) -> &[UndoLog] {
+        &self.logs[snapshot.undo_len..]
+    }
+
+    fn start_snapshot(&mut self) -> Self::Snapshot {
+        self.num_open_snapshots += 1;
+        Snapshot {
+            undo_len: self.logs.len(),
+        }
+    }
+
+    fn rollback_to<R>(&mut self, values: impl FnOnce() -> R, snapshot: Self::Snapshot)
+    where
+        R: Rollback<UndoLog>,
+    {
+        debug!("rollback_to({})", snapshot.undo_len);
+
+        if self.logs.len() > snapshot.undo_len {
+            let mut values = values();
+            while self.logs.len() > snapshot.undo_len {
+                values.reverse(self.logs.pop().unwrap());
+            }
+        }
+
+        if self.num_open_snapshots == 1 {
+            // The root snapshot. It's safe to clear the undo log because
+            // there's no snapshot further out that we might need to roll back
+            // to.
+            assert!(snapshot.undo_len == 0);
+            self.logs.clear();
+        }
+
+        self.num_open_snapshots -= 1;
+    }
+
+    fn commit(&mut self, snapshot: Self::Snapshot) {
+        debug!("commit({})", snapshot.undo_len);
+
+        if self.num_open_snapshots == 1 {
+            // The root snapshot. It's safe to clear the undo log because
+            // there's no snapshot further out that we might need to roll back
+            // to.
+            assert!(snapshot.undo_len == 0);
+            self.logs.clear();
+        }
+
+        self.num_open_snapshots -= 1;
+    }
+}
+
+#[test]
+fn separate_undo_log() {
+    let mut storage = TypeVariableStorage::default();
+    let mut undo_log = TypeVariableUndoLogs::default();
+
+    let snapshot = undo_log.start_snapshot();
+    storage.with_log(&mut undo_log).new(1);
+    storage.with_log(&mut undo_log).new(2);
+    assert_eq!(storage.len(), 2);
+
+    undo_log.rollback_to(|| &mut storage, snapshot);
+    assert_eq!(storage.len(), 0);
+}

From ee9764ba8617c2a932ef4719dee80d01b349c3af Mon Sep 17 00:00:00 2001
From: Markus Westerlind <markus.westerlind@distilnetworks.com>
Date: Tue, 7 Apr 2020 10:24:23 +0200
Subject: [PATCH 12/18] Fix review comments

---
 src/undo_log.rs            |  23 +++--
 src/unify/mod.rs           |   6 +-
 src/unify/tests.rs         | 180 +--------------------------------
 tests/external_undo_log.rs | 202 +++++++++++++++++++++++++++++++++++++
 4 files changed, 222 insertions(+), 189 deletions(-)
 create mode 100644 tests/external_undo_log.rs

diff --git a/src/undo_log.rs b/src/undo_log.rs
index 4d79e39..486d609 100644
--- a/src/undo_log.rs
+++ b/src/undo_log.rs
@@ -11,14 +11,21 @@
 /// A trait which allows actions (`T`) to be pushed which which allows the action to be undone at a
 /// later time if needed
 pub trait UndoLogs<T> {
+    /// True if a snapshot has started, false otherwise
     fn in_snapshot(&self) -> bool {
         self.num_open_snapshots() > 0
     }
-    fn num_open_snapshots(&self) -> usize {
-        0
-    }
+
+    /// How many open snapshots this undo log currently has
+    fn num_open_snapshots(&self) -> usize;
+
+    /// Pushes a new "undo item" onto the undo log. This method is invoked when some action is taken
+    /// (e.g., a variable is unified). It records the info needed to reverse that action should an
+    /// enclosing snapshot be rolleod back.
     fn push(&mut self, undo: T);
+
     fn clear(&mut self);
+
     fn extend<I>(&mut self, undos: I)
     where
         Self: Sized,
@@ -78,24 +85,24 @@ where
 {
     type Snapshot = U::Snapshot;
     fn has_changes(&self, snapshot: &Self::Snapshot) -> bool {
-        (**self).has_changes(snapshot)
+        U::has_changes(self, snapshot)
     }
     fn actions_since_snapshot(&self, snapshot: &Self::Snapshot) -> &[T] {
-        (**self).actions_since_snapshot(snapshot)
+        U::actions_since_snapshot(self, snapshot)
     }
 
     fn start_snapshot(&mut self) -> Self::Snapshot {
-        (**self).start_snapshot()
+        U::start_snapshot(self)
     }
     fn rollback_to<R>(&mut self, values: impl FnOnce() -> R, snapshot: Self::Snapshot)
     where
         R: Rollback<T>,
     {
-        (**self).rollback_to(values, snapshot)
+        U::rollback_to(self, values, snapshot)
     }
 
     fn commit(&mut self, snapshot: Self::Snapshot) {
-        (**self).commit(snapshot)
+        U::commit(self, snapshot)
     }
 }
 
diff --git a/src/unify/mod.rs b/src/unify/mod.rs
index 10a1bdb..27f14e4 100644
--- a/src/unify/mod.rs
+++ b/src/unify/mod.rs
@@ -180,7 +180,7 @@ pub struct VarValue<K: UnifyKey> {
 ///   - This implies that ordinary operations are quite a bit slower though.
 ///   - Requires the `persistent` feature be selected in your Cargo.toml file.
 #[derive(Clone, Debug, Default)]
-pub struct UnificationTable<S> {
+pub struct UnificationTable<S: UnificationStoreBase> {
     /// Indicates the current value of each key.
     values: S,
 }
@@ -248,6 +248,8 @@ where
     K: UnifyKey,
     V: sv::VecLike<Delegate<K>>,
 {
+    /// Creates a `UnificationTable` using an external `undo_log`, allowing mutating methods to be
+    /// called if `L` does not implement `UndoLogs`
     pub fn with_log<'a, L2>(
         &'a mut self,
         undo_log: L2,
@@ -269,7 +271,7 @@ where
 // other type parameter U, and we have no way to say
 // Option<U>:LatticeValue.
 
-impl<S: Default> UnificationTable<S> {
+impl<S: UnificationStoreBase + Default> UnificationTable<S> {
     pub fn new() -> Self {
         Self::default()
     }
diff --git a/src/unify/tests.rs b/src/unify/tests.rs
index 7b81b39..5665aba 100644
--- a/src/unify/tests.rs
+++ b/src/unify/tests.rs
@@ -16,14 +16,10 @@
 extern crate test;
 #[cfg(feature = "bench")]
 use self::test::Bencher;
-use snapshot_vec as sv;
 use std::cmp;
-use undo_log::{Rollback, Snapshots, UndoLogs};
 #[cfg(feature = "persistent")]
 use unify::Persistent;
-use unify::{
-    self as ut, EqUnifyValue, InPlace, InPlaceUnificationTable, NoError, UnifyKey, UnifyValue,
-};
+use unify::{EqUnifyValue, InPlace, InPlaceUnificationTable, NoError, UnifyKey, UnifyValue};
 use unify::{UnificationStore, UnificationTable};
 
 #[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)]
@@ -488,177 +484,3 @@ fn clone_table() {
         }
     }
 }
-
-enum UndoLog {
-    EqRelation(sv::UndoLog<ut::Delegate<IntKey>>),
-    Values(sv::UndoLog<i32>),
-}
-
-impl From<sv::UndoLog<ut::Delegate<IntKey>>> for UndoLog {
-    fn from(l: sv::UndoLog<ut::Delegate<IntKey>>) -> Self {
-        UndoLog::EqRelation(l)
-    }
-}
-
-impl From<sv::UndoLog<i32>> for UndoLog {
-    fn from(l: sv::UndoLog<i32>) -> Self {
-        UndoLog::Values(l)
-    }
-}
-
-impl Rollback<UndoLog> for TypeVariableStorage {
-    fn reverse(&mut self, undo: UndoLog) {
-        match undo {
-            UndoLog::EqRelation(undo) => self.eq_relations.reverse(undo),
-            UndoLog::Values(undo) => self.values.reverse(undo),
-        }
-    }
-}
-
-#[derive(Default)]
-struct TypeVariableStorage {
-    values: sv::SnapshotVecStorage<i32>,
-
-    eq_relations: ut::UnificationTableStorage<IntKey>,
-}
-
-impl TypeVariableStorage {
-    fn with_log<'a>(&'a mut self, undo_log: &'a mut TypeVariableUndoLogs) -> TypeVariableTable<'a> {
-        TypeVariableTable {
-            storage: self,
-            undo_log,
-        }
-    }
-
-    fn len(&mut self) -> usize {
-        assert_eq!(self.values.len(), self.eq_relations.len());
-        self.values.len()
-    }
-}
-
-struct TypeVariableTable<'a> {
-    storage: &'a mut TypeVariableStorage,
-
-    undo_log: &'a mut TypeVariableUndoLogs,
-}
-
-impl TypeVariableTable<'_> {
-    fn new(&mut self, i: i32) -> IntKey {
-        self.storage.values.with_log(&mut self.undo_log).push(i);
-        self.storage
-            .eq_relations
-            .with_log(&mut self.undo_log)
-            .new_key(None)
-    }
-}
-
-struct Snapshot {
-    undo_len: usize,
-}
-
-struct TypeVariableUndoLogs {
-    logs: Vec<UndoLog>,
-    num_open_snapshots: usize,
-}
-
-impl Default for TypeVariableUndoLogs {
-    fn default() -> Self {
-        Self {
-            logs: Default::default(),
-            num_open_snapshots: Default::default(),
-        }
-    }
-}
-
-impl<T> UndoLogs<T> for TypeVariableUndoLogs
-where
-    UndoLog: From<T>,
-{
-    fn num_open_snapshots(&self) -> usize {
-        self.num_open_snapshots
-    }
-    fn push(&mut self, undo: T) {
-        if self.in_snapshot() {
-            self.logs.push(undo.into())
-        }
-    }
-    fn clear(&mut self) {
-        self.logs.clear();
-        self.num_open_snapshots = 0;
-    }
-    fn extend<J>(&mut self, undos: J)
-    where
-        Self: Sized,
-        J: IntoIterator<Item = T>,
-    {
-        if self.in_snapshot() {
-            self.logs.extend(undos.into_iter().map(UndoLog::from))
-        }
-    }
-}
-
-impl Snapshots<UndoLog> for TypeVariableUndoLogs {
-    type Snapshot = Snapshot;
-    fn actions_since_snapshot(&self, snapshot: &Self::Snapshot) -> &[UndoLog] {
-        &self.logs[snapshot.undo_len..]
-    }
-
-    fn start_snapshot(&mut self) -> Self::Snapshot {
-        self.num_open_snapshots += 1;
-        Snapshot {
-            undo_len: self.logs.len(),
-        }
-    }
-
-    fn rollback_to<R>(&mut self, values: impl FnOnce() -> R, snapshot: Self::Snapshot)
-    where
-        R: Rollback<UndoLog>,
-    {
-        debug!("rollback_to({})", snapshot.undo_len);
-
-        if self.logs.len() > snapshot.undo_len {
-            let mut values = values();
-            while self.logs.len() > snapshot.undo_len {
-                values.reverse(self.logs.pop().unwrap());
-            }
-        }
-
-        if self.num_open_snapshots == 1 {
-            // The root snapshot. It's safe to clear the undo log because
-            // there's no snapshot further out that we might need to roll back
-            // to.
-            assert!(snapshot.undo_len == 0);
-            self.logs.clear();
-        }
-
-        self.num_open_snapshots -= 1;
-    }
-
-    fn commit(&mut self, snapshot: Self::Snapshot) {
-        debug!("commit({})", snapshot.undo_len);
-
-        if self.num_open_snapshots == 1 {
-            // The root snapshot. It's safe to clear the undo log because
-            // there's no snapshot further out that we might need to roll back
-            // to.
-            assert!(snapshot.undo_len == 0);
-            self.logs.clear();
-        }
-
-        self.num_open_snapshots -= 1;
-    }
-}
-
-#[test]
-fn separate_undo_log() {
-    let mut storage = TypeVariableStorage::default();
-    let mut undo_log = TypeVariableUndoLogs::default();
-
-    let snapshot = undo_log.start_snapshot();
-    storage.with_log(&mut undo_log).new(1);
-    storage.with_log(&mut undo_log).new(2);
-    assert_eq!(storage.len(), 2);
-
-    undo_log.rollback_to(|| &mut storage, snapshot);
-    assert_eq!(storage.len(), 0);
-}
diff --git a/tests/external_undo_log.rs b/tests/external_undo_log.rs
new file mode 100644
index 0000000..2537826
--- /dev/null
+++ b/tests/external_undo_log.rs
@@ -0,0 +1,202 @@
+#[macro_use]
+extern crate log;
+extern crate ena;
+
+use ena::{
+    snapshot_vec as sv,
+    undo_log::{Rollback, Snapshots, UndoLogs},
+    unify::{self as ut, EqUnifyValue, UnifyKey},
+};
+
+#[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)]
+struct IntKey(u32);
+
+impl UnifyKey for IntKey {
+    type Value = Option<IntKey>;
+    fn index(&self) -> u32 {
+        self.0
+    }
+    fn from_index(u: u32) -> IntKey {
+        IntKey(u)
+    }
+    fn tag() -> &'static str {
+        "IntKey"
+    }
+}
+
+impl EqUnifyValue for IntKey {}
+
+enum UndoLog {
+    EqRelation(sv::UndoLog<ut::Delegate<IntKey>>),
+    Values(sv::UndoLog<i32>),
+}
+
+impl From<sv::UndoLog<ut::Delegate<IntKey>>> for UndoLog {
+    fn from(l: sv::UndoLog<ut::Delegate<IntKey>>) -> Self {
+        UndoLog::EqRelation(l)
+    }
+}
+
+impl From<sv::UndoLog<i32>> for UndoLog {
+    fn from(l: sv::UndoLog<i32>) -> Self {
+        UndoLog::Values(l)
+    }
+}
+
+impl Rollback<UndoLog> for TypeVariableStorage {
+    fn reverse(&mut self, undo: UndoLog) {
+        match undo {
+            UndoLog::EqRelation(undo) => self.eq_relations.reverse(undo),
+            UndoLog::Values(undo) => self.values.reverse(undo),
+        }
+    }
+}
+
+#[derive(Default)]
+struct TypeVariableStorage {
+    values: sv::SnapshotVecStorage<i32>,
+
+    eq_relations: ut::UnificationTableStorage<IntKey>,
+}
+
+impl TypeVariableStorage {
+    fn with_log<'a>(&'a mut self, undo_log: &'a mut TypeVariableUndoLogs) -> TypeVariableTable<'a> {
+        TypeVariableTable {
+            storage: self,
+            undo_log,
+        }
+    }
+
+    fn len(&mut self) -> usize {
+        assert_eq!(self.values.len(), self.eq_relations.len());
+        self.values.len()
+    }
+}
+
+struct TypeVariableTable<'a> {
+    storage: &'a mut TypeVariableStorage,
+
+    undo_log: &'a mut TypeVariableUndoLogs,
+}
+
+impl TypeVariableTable<'_> {
+    fn new(&mut self, i: i32) -> IntKey {
+        self.storage.values.with_log(&mut self.undo_log).push(i);
+        self.storage
+            .eq_relations
+            .with_log(&mut self.undo_log)
+            .new_key(None)
+    }
+}
+
+struct Snapshot {
+    undo_len: usize,
+}
+
+struct TypeVariableUndoLogs {
+    logs: Vec<UndoLog>,
+    num_open_snapshots: usize,
+}
+
+impl Default for TypeVariableUndoLogs {
+    fn default() -> Self {
+        Self {
+            logs: Default::default(),
+            num_open_snapshots: Default::default(),
+        }
+    }
+}
+
+impl<T> UndoLogs<T> for TypeVariableUndoLogs
+where
+    UndoLog: From<T>,
+{
+    fn num_open_snapshots(&self) -> usize {
+        self.num_open_snapshots
+    }
+    fn push(&mut self, undo: T) {
+        if self.in_snapshot() {
+            self.logs.push(undo.into())
+        }
+    }
+    fn clear(&mut self) {
+        self.logs.clear();
+        self.num_open_snapshots = 0;
+    }
+    fn extend<J>(&mut self, undos: J)
+    where
+        Self: Sized,
+        J: IntoIterator<Item = T>,
+    {
+        if self.in_snapshot() {
+            self.logs.extend(undos.into_iter().map(UndoLog::from))
+        }
+    }
+}
+
+impl Snapshots<UndoLog> for TypeVariableUndoLogs {
+    type Snapshot = Snapshot;
+    fn actions_since_snapshot(&self, snapshot: &Self::Snapshot) -> &[UndoLog] {
+        &self.logs[snapshot.undo_len..]
+    }
+
+    fn start_snapshot(&mut self) -> Self::Snapshot {
+        self.num_open_snapshots += 1;
+        Snapshot {
+            undo_len: self.logs.len(),
+        }
+    }
+
+    fn rollback_to<R>(&mut self, values: impl FnOnce() -> R, snapshot: Self::Snapshot)
+    where
+        R: Rollback<UndoLog>,
+    {
+        debug!("rollback_to({})", snapshot.undo_len);
+
+        if self.logs.len() > snapshot.undo_len {
+            let mut values = values();
+            while self.logs.len() > snapshot.undo_len {
+                values.reverse(self.logs.pop().unwrap());
+            }
+        }
+
+        if self.num_open_snapshots == 1 {
+            // The root snapshot. It's safe to clear the undo log because
+            // there's no snapshot further out that we might need to roll back
+            // to.
+            assert!(snapshot.undo_len == 0);
+            self.logs.clear();
+        }
+
+        self.num_open_snapshots -= 1;
+    }
+
+    fn commit(&mut self, snapshot: Self::Snapshot) {
+        debug!("commit({})", snapshot.undo_len);
+
+        if self.num_open_snapshots == 1 {
+            // The root snapshot. It's safe to clear the undo log because
+            // there's no snapshot further out that we might need to roll back
+            // to.
+            assert!(snapshot.undo_len == 0);
+            self.logs.clear();
+        }
+
+        self.num_open_snapshots -= 1;
+    }
+}
+
+/// Tests that a undo log stored externally can be used with TypeVariableTable
+#[test]
+fn external_undo_log() {
+    let mut storage = TypeVariableStorage::default();
+    let mut undo_log = TypeVariableUndoLogs::default();
+
+    let snapshot = undo_log.start_snapshot();
+    storage.with_log(&mut undo_log).new(1);
+    storage.with_log(&mut undo_log).new(2);
+    assert_eq!(storage.len(), 2);
+
+    undo_log.rollback_to(|| &mut storage, snapshot);
+    assert_eq!(storage.len(), 0);
+}

From 7d7d5e950ef6546bb2752432d8d9eee8a6c537ef Mon Sep 17 00:00:00 2001
From: Markus Westerlind <markus.westerlind@distilnetworks.com>
Date: Tue, 7 Apr 2020 10:37:10 +0200
Subject: [PATCH 13/18] Fix review comments

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

diff --git a/src/undo_log.rs b/src/undo_log.rs
index 486d609..5da028d 100644
--- a/src/undo_log.rs
+++ b/src/undo_log.rs
@@ -24,8 +24,10 @@ pub trait UndoLogs<T> {
     /// enclosing snapshot be rolleod back.
     fn push(&mut self, undo: T);
 
+    /// Removes all items from the undo log.
     fn clear(&mut self);
 
+    /// Extends the undo log with many undos.
     fn extend<I>(&mut self, undos: I)
     where
         Self: Sized,
@@ -40,23 +42,23 @@ where
     U: UndoLogs<T>,
 {
     fn in_snapshot(&self) -> bool {
-        (**self).in_snapshot()
+        U::in_snapshot(self)
     }
     fn num_open_snapshots(&self) -> usize {
-        (**self).num_open_snapshots()
+        U::num_open_snapshots(self)
     }
     fn push(&mut self, undo: T) {
-        (**self).push(undo)
+        U::push(self, undo)
     }
     fn clear(&mut self) {
-        (**self).clear();
+        U::clear(self);
     }
     fn extend<I>(&mut self, undos: I)
     where
         Self: Sized,
         I: IntoIterator<Item = T>,
     {
-        (**self).extend(undos)
+        U::extend(self, undos)
     }
 }
 

From beeafed2f89d3018b4fd56d886a68248667f11f8 Mon Sep 17 00:00:00 2001
From: Markus Westerlind <markus.westerlind@distilnetworks.com>
Date: Tue, 7 Apr 2020 10:46:39 +0200
Subject: [PATCH 14/18] Fix review comments

---
 src/undo_log.rs | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/src/undo_log.rs b/src/undo_log.rs
index 5da028d..f5b5178 100644
--- a/src/undo_log.rs
+++ b/src/undo_log.rs
@@ -62,22 +62,33 @@ where
     }
 }
 
-/// A trait which allows snapshots to be done at specific points. Each snapshot can then be used to
+/// A trait which extends `UndoLogs` to allow snapshots to be done at specific points. Each snapshot can then be used to
 /// rollback any changes to an underlying data structures if they were not desirable.
 ///
 /// Each snapshot must be consumed linearly with either `rollback_to` or `commit`.
 pub trait Snapshots<T>: UndoLogs<T> {
     type Snapshot;
+
+    /// Returns true if `self` has made any changes since snapshot started.
     fn has_changes(&self, snapshot: &Self::Snapshot) -> bool {
         !self.actions_since_snapshot(snapshot).is_empty()
     }
+
+    /// Returns the slice of actions that were taken since the snapshot began.
     fn actions_since_snapshot(&self, snapshot: &Self::Snapshot) -> &[T];
 
+    /// Starts a new snapshot. That snapshot must eventually either be committed via a call to
+    /// commit or rollback via rollback_to. Snapshots can be nested (i.e., you can start a snapshot
+    /// whilst another snapshot is in progress) but you must then commit or rollback the inner
+    /// snapshot before attempting to commit or rollback the outer snapshot.
     fn start_snapshot(&mut self) -> Self::Snapshot;
-    fn rollback_to<R>(&mut self, values: impl FnOnce() -> R, snapshot: Self::Snapshot)
+
+    /// Rollback (undo) the changes made to `storage` since the snapshot.
+    fn rollback_to<R>(&mut self, storage: impl FnOnce() -> R, snapshot: Self::Snapshot)
     where
         R: Rollback<T>;
 
+    /// Commit: keep the changes that have been made since the snapshot began
     fn commit(&mut self, snapshot: Self::Snapshot);
 }
 
@@ -96,11 +107,11 @@ where
     fn start_snapshot(&mut self) -> Self::Snapshot {
         U::start_snapshot(self)
     }
-    fn rollback_to<R>(&mut self, values: impl FnOnce() -> R, snapshot: Self::Snapshot)
+    fn rollback_to<R>(&mut self, storage: impl FnOnce() -> R, snapshot: Self::Snapshot)
     where
         R: Rollback<T>,
     {
-        U::rollback_to(self, values, snapshot)
+        U::rollback_to(self, storage, snapshot)
     }
 
     fn commit(&mut self, snapshot: Self::Snapshot) {
@@ -213,7 +224,7 @@ impl<T> std::ops::Index<usize> for VecLog<T> {
     }
 }
 
-/// A trait implemented for types which can be rolled back using actions of type `U`.
+/// A trait implemented for storage types (like `SnapshotVecStorage`) which can be rolled back using actions of type `U`.
 pub trait Rollback<U> {
     fn reverse(&mut self, undo: U);
 }

From 9568b3b7225d1c7ceef04ad072eb0afc862e63ed Mon Sep 17 00:00:00 2001
From: Markus Westerlind <markus.westerlind@distilnetworks.com>
Date: Tue, 7 Apr 2020 10:47:48 +0200
Subject: [PATCH 15/18] Fix review comments

---
 src/snapshot_vec.rs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/snapshot_vec.rs b/src/snapshot_vec.rs
index 235d629..62dbd59 100644
--- a/src/snapshot_vec.rs
+++ b/src/snapshot_vec.rs
@@ -164,7 +164,8 @@ impl<D: SnapshotVecDelegate, V: VecLike<D> + Default, L: Default> SnapshotVec<D,
 }
 
 impl<D: SnapshotVecDelegate, V: VecLike<D>, L> SnapshotVec<D, V, L> {
-    /// Creates a `SnapshotVec` using the `undo_log`, allowing mutating methods to be called
+    /// Creates a `SnapshotVec` using the `undo_log`, allowing mutating methods to be
+    /// called if `L` does not implement `UndoLogs`
     pub fn with_log<'a, L2>(&'a mut self, undo_log: L2) -> SnapshotVec<D, &'a mut V, L2>
     where
         L2: UndoLogs<UndoLog<D>>,

From b1934a178944f6b86614afc00bdd9fe929850aef Mon Sep 17 00:00:00 2001
From: Markus Westerlind <markus.westerlind@distilnetworks.com>
Date: Tue, 7 Apr 2020 10:51:05 +0200
Subject: [PATCH 16/18] refactor: Restrict with_log to the Storage types

Not strictly necessary, but it documents things better and may make some
mistakes impossible
---
 src/snapshot_vec.rs | 13 +++++++------
 src/unify/mod.rs    | 12 +++++-------
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/src/snapshot_vec.rs b/src/snapshot_vec.rs
index 62dbd59..efde49a 100644
--- a/src/snapshot_vec.rs
+++ b/src/snapshot_vec.rs
@@ -163,13 +163,14 @@ impl<D: SnapshotVecDelegate, V: VecLike<D> + Default, L: Default> SnapshotVec<D,
     }
 }
 
-impl<D: SnapshotVecDelegate, V: VecLike<D>, L> SnapshotVec<D, V, L> {
-    /// Creates a `SnapshotVec` using the `undo_log`, allowing mutating methods to be
-    /// called if `L` does not implement `UndoLogs`
-    pub fn with_log<'a, L2>(&'a mut self, undo_log: L2) -> SnapshotVec<D, &'a mut V, L2>
+impl<D: SnapshotVecDelegate> SnapshotVecStorage<D> {
+    /// Creates a `SnapshotVec` using the `undo_log`, allowing mutating methods to be called
+    pub fn with_log<'a, L>(
+        &'a mut self,
+        undo_log: L,
+    ) -> SnapshotVec<D, &'a mut Vec<<D as SnapshotVecDelegate>::Value>, L>
     where
-        L2: UndoLogs<UndoLog<D>>,
-        &'a mut V: VecLike<D>,
+        L: UndoLogs<UndoLog<D>>,
     {
         SnapshotVec {
             values: &mut self.values,
diff --git a/src/unify/mod.rs b/src/unify/mod.rs
index 27f14e4..a26d699 100644
--- a/src/unify/mod.rs
+++ b/src/unify/mod.rs
@@ -243,20 +243,18 @@ impl<K: UnifyKey> VarValue<K> {
         }
     }
 }
-impl<K, V, L> UnificationTable<InPlace<K, V, L>>
+impl<K> UnificationTableStorage<K>
 where
     K: UnifyKey,
-    V: sv::VecLike<Delegate<K>>,
 {
     /// Creates a `UnificationTable` using an external `undo_log`, allowing mutating methods to be
     /// called if `L` does not implement `UndoLogs`
-    pub fn with_log<'a, L2>(
+    pub fn with_log<'a, L>(
         &'a mut self,
-        undo_log: L2,
-    ) -> UnificationTable<InPlace<K, &'a mut V, L2>>
+        undo_log: L,
+    ) -> UnificationTable<InPlace<K, &'a mut UnificationStorage<K>, L>>
     where
-        L2: UndoLogs<sv::UndoLog<Delegate<K>>>,
-        &'a mut V: sv::VecLike<Delegate<K>>,
+        L: UndoLogs<sv::UndoLog<Delegate<K>>>,
     {
         UnificationTable {
             values: InPlace {

From c0ab90a19e28789677b2915ac34d28f5008386b4 Mon Sep 17 00:00:00 2001
From: Markus Westerlind <markus.westerlind@distilnetworks.com>
Date: Tue, 7 Apr 2020 10:52:52 +0200
Subject: [PATCH 17/18] refactor: Clarify reverse forwarding

---
 src/undo_log.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/undo_log.rs b/src/undo_log.rs
index f5b5178..f7bbd52 100644
--- a/src/undo_log.rs
+++ b/src/undo_log.rs
@@ -234,7 +234,7 @@ where
     T: Rollback<U>,
 {
     fn reverse(&mut self, undo: U) {
-        (**self).reverse(undo)
+        T::reverse(self, undo)
     }
 }
 

From 7e3b49bdcbb312d27496789f407cd8f90f8f6505 Mon Sep 17 00:00:00 2001
From: Markus Westerlind <markus.westerlind@distilnetworks.com>
Date: Tue, 7 Apr 2020 17:49:09 +0200
Subject: [PATCH 18/18] doc: Explain the opaqueness of `T` in UndoLogs<T>

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

diff --git a/src/undo_log.rs b/src/undo_log.rs
index f7bbd52..93c3803 100644
--- a/src/undo_log.rs
+++ b/src/undo_log.rs
@@ -8,8 +8,11 @@
 //! Since the `*Storage` variants do not have an undo log `with_log` must be called with the
 //! unified log before any mutating actions.
 
-/// A trait which allows actions (`T`) to be pushed which which allows the action to be undone at a
-/// later time if needed
+/// A trait which allows undo actions (`T`) to be pushed which can be used to rollback actio at a
+/// later time if needed.
+///
+/// The undo actions themselves are opaque to `UndoLogs`, only specified `Rollback` implementations
+/// need to know what an action is and how to reverse it.
 pub trait UndoLogs<T> {
     /// True if a snapshot has started, false otherwise
     fn in_snapshot(&self) -> bool {