Apply suggestions from code review

Co-authored-by: Chase Wilson <[email protected]>

lib/hashset.dl

@@ -121,7 +121,7 @@ function find(s: HashSet<'A>, f: function('A): bool): Option<'A> {
     None
 }
 
-/* Returns a vector containing only those elements in `s` that satisfy predicate
+/* Returns a set containing only those elements in `s` that satisfy predicate
  * `f`. */
 function filter(s: HashSet<'A>, f: function('A): bool): HashSet<'A> {
     var res = hashset_empty();

lib/hashset.rs

@@ -194,22 +194,27 @@ impl<T: Hash + Eq + Clone> HashSet<T> {
         set.insert(x);
         HashSet::from(set)
     }
+
     pub fn insert(&mut self, x: T) -> Option<T> {
         self.set.insert(x)
     }
+
     pub fn remove<BT: ?Sized>(&mut self, x: &BT) -> Option<T>
     where
         BT: Hash + Eq,
         T: Borrow<BT>,
     {
         self.set.remove(x)
     }
+
     pub fn without(&self, x: &T) -> Self {
         HashSet::from(self.set.without(x))
     }
+
     pub fn update(&self, x: T) -> Self {
         HashSet::from(self.set.update(x))
     }
+
     pub fn union(self, other: Self) -> Self {
         // `im::HashSet` implements union by adding elements from `other` to
         // `self`.  This is inefficient when `other` is much
@@ -289,11 +294,7 @@ impl<T: IntoRecord + Hash + Eq + Clone + Ord> IntoRecord for HashSet<T> {
 impl<T: FromRecord + Hash + Eq + Clone + Ord> Mutator<HashSet<T>> for Record {
     fn mutate(&self, set: &mut HashSet<T>) -> StdResult<(), String> {
         let upd = <HashSet<T>>::from_record(self)?;
-        for v in upd.into_iter() {
-            if set.remove(&v).is_none() {
-                set.insert(v);
-            }
-        }
+        *set = HashSet::from(set.set.clone().symmetric_difference(upd.set));
         Ok(())
     }
 }

rust/template/cmd_parser/lib.rs

@@ -28,7 +28,8 @@ where
     let mut buf: Vec<u8> = Vec::new();
 
     let istty = unsafe {
-        libc::isatty(/*libc::STDIN_FILENO*/ 0)
+        // libc::STDIN_FILENO
+        libc::isatty(0)
     } != 0;
     let mut input = if istty {
         let mut rl = Editor::<()>::new();

rust/template/differential_datalog/src/program/worker.rs

@@ -451,6 +451,8 @@ impl<'a> DDlogWorker<'a> {
             // with `Enabled`.  `Enabled` contains a single record (an empty tuple) as long as
             // the program is running.  We retract this record on shutdown, hopefully enforcing
             // quiescing the dataflow.
+            // Note: this trick won't be needed once DD supports proactive termination:
+            // https://github.com/TimelyDataflow/timely-dataflow/issues/306
             let (enabled_session, enabled_collection) = outer.new_collection::<(),Weight>();
             let enabled_arrangement = enabled_collection.arrange_by_self();
 
@@ -469,7 +471,7 @@ impl<'a> DDlogWorker<'a> {
                                 (),
                                 (),
                                 (),
-                                )
+                            )
                             );
                 (drel.id, (drel.rel_id, v, vcol))
             }).collect();

test/datalog_tests/hashset_test.dat

@@ -1,2 +1,22 @@
 dump hashset_test::SetTransforms;
 dump hashset_test::SetFolds;
+
+start;
+
+insert hashset_test::TestSetMutateI(1, [1,2,3,4,5]),
+insert hashset_test::TestSetMutateI(2, [1,2,3,4,5]),
+insert hashset_test::TestSetMutateI(3, [1,2,3,4,5]),
+insert hashset_test::TestSetMutateI(4, [1,2,3,4,5]),
+insert hashset_test::TestSetMutateI(5, [1,2,3,4,5]),
+
+commit dump_changes;
+
+start;
+
+modify hashset_test::TestSetMutateI 1 <- hashset_test::TestSetMutateI{1, [1,2,3,4,5]},
+modify hashset_test::TestSetMutateI 2 <- hashset_test::TestSetMutateI{2, [6,7,8,9,10]},
+modify hashset_test::TestSetMutateI 3 <- hashset_test::TestSetMutateI{3, [1,2,3]},
+modify hashset_test::TestSetMutateI 4 <- hashset_test::TestSetMutateI{4, [1,3,5,7,9]},
+modify hashset_test::TestSetMutateI 5 <- hashset_test::TestSetMutateI{5, []},
+
+commit dump_changes;

test/datalog_tests/hashset_test.dl

@@ -80,3 +80,9 @@ SetFolds(
     test_set().fold(|a, x| a + x.1.fold(|b,y| b + y, 0), 0)
 ).
 
+input relation TestSetMutateI(k: usize, v: HashSet<usize>)
+primary key (x) x.k
+
+output relation TestSetMutateO(k: usize, v: HashSet<usize>)
+
+TestSetMutateO(k, v) :- TestSetMutateI(k, v).

test/datalog_tests/hashset_test.dump.expected

@@ -8,3 +8,18 @@ hashset_test::SetTransforms{.description = "find(contains(3))", .s = [(1, [2, 3,
 hashset_test::SetTransforms{.description = "map(push 100)", .s = [(1, [2, 3, 4, 100]), (3, [6, 7, 8, 100]), (2, [4, 5, 6, 100]), (0, [0, 1, 2, 100]), (4, [8, 9, 10, 100])]}
 hashset_test::SetFolds{.description = "fold(+)", .a = 10}
 hashset_test::SetFolds{.description = "fold(fold(+))", .a = 75}
+hashset_test::TestSetMutateO:
+hashset_test::TestSetMutateO{.k = 1, .v = [4, 2, 1, 3, 5]}: +1
+hashset_test::TestSetMutateO{.k = 2, .v = [4, 2, 1, 3, 5]}: +1
+hashset_test::TestSetMutateO{.k = 3, .v = [4, 2, 1, 3, 5]}: +1
+hashset_test::TestSetMutateO{.k = 4, .v = [4, 2, 1, 3, 5]}: +1
+hashset_test::TestSetMutateO{.k = 5, .v = [4, 2, 1, 3, 5]}: +1
+hashset_test::TestSetMutateO:
+hashset_test::TestSetMutateO{.k = 1, .v = []}: +1
+hashset_test::TestSetMutateO{.k = 1, .v = [4, 2, 1, 3, 5]}: -1
+hashset_test::TestSetMutateO{.k = 2, .v = [4, 2, 1, 3, 5]}: -1
+hashset_test::TestSetMutateO{.k = 2, .v = [10, 6, 8, 9, 4, 2, 1, 3, 7, 5]}: +1
+hashset_test::TestSetMutateO{.k = 3, .v = [4, 5]}: +1
+hashset_test::TestSetMutateO{.k = 3, .v = [4, 2, 1, 3, 5]}: -1
+hashset_test::TestSetMutateO{.k = 4, .v = [9, 4, 2, 7]}: +1
+hashset_test::TestSetMutateO{.k = 4, .v = [4, 2, 1, 3, 5]}: -1