fix: Should properly look for element when removing (#29)

bluefireteam · Apr 16, 2024 · ed2d5a3 · ed2d5a3
1 parent cf77183
commit ed2d5a3
Show file tree

Hide file tree

Showing 2 changed files with 14 additions and 4 deletions.
diff --git a/lib/ordered_set.dart b/lib/ordered_set.dart
@@ -79,8 +79,8 @@ class OrderedSet<E> extends IterableMixin<E> {
   /// added.
   bool add(E e) {
     final elementSet = {e};
-    var added = false;
-    final isRootSet = added = _backingSet.add(elementSet);
+    var added = _backingSet.add(elementSet);
+    final isRootSet = added;
     if (!isRootSet) {
       added = _backingSet.lookup(elementSet)!.add(e);
     }
@@ -137,7 +137,7 @@ class OrderedSet<E> extends IterableMixin<E> {
   ///     set.removeWhere((a) => a == e);
   ///
   bool remove(E e) {
-    var bucket = _backingSet.lookup([e]);
+    var bucket = _backingSet.lookup({e});
     if (bucket == null || !bucket.contains(e)) {
       // We need a fallback in case [e] has changed and it's no longer found by
       // lookup. Note: changing priorities will leave the splay set on an

diff --git a/test/ordered_set_test.dart b/test/ordered_set_test.dart
@@ -320,14 +320,24 @@ void main() {
 
       test('sorts after remove', () {
         final orderedSet = OrderedSet<int>();
-        // The initial elements must be in order.
+        // The initial elements must be in order to reproduce the issue.
         orderedSet.addAll([1, 3, 4]);
         expect(orderedSet.toList().join(), '134');
         expect(orderedSet.remove(4), true);
         expect(orderedSet.toList().join(), '13');
         expect(orderedSet.add(2), true);
         expect(orderedSet.toList().join(), '123');
       });
+
+      test('correct order after remove', () {
+        final set = OrderedSet<int>();
+        set.add(10);
+        set.add(9);
+        set.remove(10);
+        set.add(11);
+        set.add(8);
+        expect(set.toList(), [8, 9, 11]);
+      });
     });
 
     group('rebalancing', () {