AK: Add support for removing SinglyLinkedList nodes during iteration

This commit also fixes the now-broken usage of SinglyLinkedList::remove
in the Piano application.

AK/SinglyLinkedList.h

@@ -21,24 +21,45 @@ public:
     bool operator!=(const SinglyLinkedListIterator& other) const { return m_node != other.m_node; }
     SinglyLinkedListIterator& operator++()
     {
-        m_prev = m_node;
+        if (m_removed)
-        m_node = m_node->next;
+            m_removed = false;
+        else
+            m_prev = m_node;
+        m_node = m_next;
+        if (m_next)
+            m_next = m_next->next;
         return *this;
     }
-    ElementType& operator*() { return m_node->value; }
+    ElementType& operator*()
-    ElementType* operator->() { return &m_node->value; }
+    {
+        VERIFY(!m_removed);
+        return m_node->value;
+    }
+    ElementType* operator->()
+    {
+        VERIFY(!m_removed);
+        return &m_node->value;
+    }
     bool is_end() const { return !m_node; }
     bool is_begin() const { return !m_prev; }
+    void remove(ListType& list)
+    {
+        m_removed = true;
+        list.remove(*this);
+    };
 
 private:
     friend ListType;
     explicit SinglyLinkedListIterator(typename ListType::Node* node, typename ListType::Node* prev = nullptr)
         : m_node(node)
         , m_prev(prev)
+        , m_next(node ? node->next : nullptr)
     {
     }
     typename ListType::Node* m_node { nullptr };
     typename ListType::Node* m_prev { nullptr };
+    typename ListType::Node* m_next { nullptr };
+    bool m_removed { false };
 };
 
 template<typename T>
@@ -165,18 +186,6 @@ public:
         return find_if([&](auto& other) { return Traits<T>::equals(value, other); });
     }
 
-    void remove(Iterator iterator)
-    {
-        VERIFY(!iterator.is_end());
-        if (m_head == iterator.m_node)
-            m_head = iterator.m_node->next;
-        if (m_tail == iterator.m_node)
-            m_tail = iterator.m_prev;
-        if (iterator.m_prev)
-            iterator.m_prev->next = iterator.m_node->next;
-        delete iterator.m_node;
-    }
-
     template<typename U = T>
     void insert_before(Iterator iterator, U&& value)
     {
@@ -206,6 +215,18 @@ public:
     }
 
 private:
+    void remove(Iterator& iterator)
+    {
+        VERIFY(!iterator.is_end());
+        if (m_head == iterator.m_node)
+            m_head = iterator.m_node->next;
+        if (m_tail == iterator.m_node)
+            m_tail = iterator.m_prev;
+        if (iterator.m_prev)
+            iterator.m_prev->next = iterator.m_node->next;
+        delete iterator.m_node;
+    }
+
     Node* head() { return m_head; }
     const Node* head() const { return m_head; }
 

Tests/AK/TestSinglyLinkedList.cpp

@@ -59,3 +59,14 @@ TEST_CASE(should_find_const_with_predicate)
 
     EXPECT_EQ(sut.end(), sut.find_if([](const auto v) { return v == 42; }));
 }
+
+TEST_CASE(removal_during_iteration)
+{
+    auto list = make_list();
+    auto size = list.size_slow();
+
+    for (auto it = list.begin(); it != list.end(); ++it, --size) {
+        VERIFY(list.size_slow() == size);
+        it.remove(list);
+    }
+}

Userland/Applications/Piano/Track.cpp

@@ -30,14 +30,14 @@ void Track::fill_sample(Sample& sample)
     Audio::Frame new_sample;
 
     for (size_t note = 0; note < note_count; ++note) {
-        if (!m_roll_iters[note].is_end()) {
+        if (!m_roll_iterators[note].is_end()) {
-            if (m_roll_iters[note]->on_sample == m_time) {
+            if (m_roll_iterators[note]->on_sample == m_time) {
                 set_note(note, On);
-            } else if (m_roll_iters[note]->off_sample == m_time) {
+            } else if (m_roll_iterators[note]->off_sample == m_time) {
                 set_note(note, Off);
-                ++m_roll_iters[note];
+                ++m_roll_iterators[note];
-                if (m_roll_iters[note].is_end())
+                if (m_roll_iterators[note].is_end())
-                    m_roll_iters[note] = m_roll_notes[note].begin();
+                    m_roll_iterators[note] = m_roll_notes[note].begin();
             }
         }
 
@@ -118,7 +118,7 @@ void Track::reset()
     memset(m_envelope, 0, sizeof(m_envelope));
 
     for (size_t note = 0; note < note_count; ++note)
-        m_roll_iters[note] = m_roll_notes[note].begin();
+        m_roll_iterators[note] = m_roll_notes[note].begin();
 }
 
 String Track::set_recorded_sample(const StringView& path)
@@ -259,9 +259,9 @@ void Track::sync_roll(int note)
 {
     auto it = m_roll_notes[note].find_if([&](auto& roll_note) { return roll_note.off_sample > m_time; });
     if (it.is_end())
-        m_roll_iters[note] = m_roll_notes[note].begin();
+        m_roll_iterators[note] = m_roll_notes[note].begin();
     else
-        m_roll_iters[note] = it;
+        m_roll_iterators[note] = it;
 }
 
 void Track::set_roll_note(int note, u32 on_sample, u32 off_sample)
@@ -281,14 +281,14 @@ void Track::set_roll_note(int note, u32 on_sample, u32 off_sample)
         if (it->on_sample <= new_roll_note.on_sample && it->off_sample >= new_roll_note.on_sample) {
             if (m_time >= it->on_sample && m_time <= it->off_sample)
                 set_note(note, Off);
-            m_roll_notes[note].remove(it);
+            it.remove(m_roll_notes[note]);
             sync_roll(note);
             return;
         }
         if ((new_roll_note.on_sample == 0 || it->on_sample >= new_roll_note.on_sample - 1) && it->on_sample <= new_roll_note.off_sample) {
             if (m_time >= new_roll_note.off_sample && m_time <= it->off_sample)
                 set_note(note, Off);
-            m_roll_notes[note].remove(it);
+            it.remove(m_roll_notes[note]);
             it = m_roll_notes[note].begin();
             continue;
         }

Userland/Applications/Piano/Track.h

@@ -86,5 +86,5 @@ private:
     const u32& m_time;
 
     SinglyLinkedList<RollNote> m_roll_notes[note_count];
-    RollIter m_roll_iters[note_count];
+    RollIter m_roll_iterators[note_count];
 };