Fix review comments

Author: Markus Westerlind

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)
     }
 }
 

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()
     }

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);
-}