IntrusiveList: add move constructor/assignment (#19280)

* IntrusiveList: add move constructor/assignment * Apply suggestions from code review Co-authored-by: Boris Zbarsky <[email protected]> * Fix CI: style & lint Co-authored-by: Boris Zbarsky <[email protected]>
project-chip · Jun 8, 2022 · c8fa512 · c8fa512
1 parent b8e9ab1
commit c8fa512
Show file tree

Hide file tree

Showing 2 changed files with 115 additions and 0 deletions.
diff --git a/src/lib/support/IntrusiveList.h b/src/lib/support/IntrusiveList.h
@@ -31,12 +31,30 @@ class IntrusiveListNodeBase
     IntrusiveListNodeBase() : mPrev(nullptr), mNext(nullptr) {}
     ~IntrusiveListNodeBase() { VerifyOrDie(!IsInList()); }
 
+    // Note: The copy construct/assignment is not provided because the list node state is not copyable.
+    //       The move construct/assignment is not provided because all modifications to the list shall go through the list object.
+    IntrusiveListNodeBase(const IntrusiveListNodeBase &) = delete;
+    IntrusiveListNodeBase & operator=(const IntrusiveListNodeBase &) = delete;
+    IntrusiveListNodeBase(IntrusiveListNodeBase &&)                  = delete;
+    IntrusiveListNodeBase & operator=(IntrusiveListNodeBase &&) = delete;
+
     bool IsInList() const { return (mPrev != nullptr && mNext != nullptr); }
 
 private:
     friend class IntrusiveListBase;
     IntrusiveListNodeBase(IntrusiveListNodeBase * prev, IntrusiveListNodeBase * next) : mPrev(prev), mNext(next) {}
 
+    void TakePlaceOf(const IntrusiveListNodeBase * that)
+    {
+        // prerequisite `that` is in a list
+        // `this` will take place of the position of `that`.
+        // `that` will be emptied by the caller after this function
+        mPrev        = that->mPrev;
+        mNext        = that->mNext;
+        mPrev->mNext = this;
+        mNext->mPrev = this;
+    }
+
     void Prepend(IntrusiveListNodeBase * node)
     {
         VerifyOrDie(IsInList());
@@ -195,6 +213,27 @@ class IntrusiveListBase
         mNode.Remove();
     }
 
+    IntrusiveListBase(const IntrusiveListBase &) = delete;
+    IntrusiveListBase & operator=(const IntrusiveListBase &) = delete;
+
+    IntrusiveListBase(IntrusiveListBase && that) : mNode(&mNode, &mNode) { *this = std::move(that); }
+
+    IntrusiveListBase & operator=(IntrusiveListBase && that)
+    {
+        VerifyOrDie(Empty());
+        if (!that.Empty())
+        {
+            mNode.TakePlaceOf(&that.mNode);
+            that.mNode.mNext = &that.mNode;
+            that.mNode.mPrev = &that.mNode;
+        }
+        else
+        {
+            // Do nothing here if that is empty, because there is a prerequisite that `this` is empty.
+        }
+        return *this;
+    }
+
     ConstIteratorBase begin() const { return ConstIteratorBase(mNode.mNext); }
     ConstIteratorBase end() const { return ConstIteratorBase(&mNode); }
     IteratorBase begin() { return IteratorBase(mNode.mNext); }
@@ -207,6 +246,16 @@ class IntrusiveListBase
     void InsertAfter(IteratorBase pos, IntrusiveListNodeBase * node) { pos.mCurrent->Append(node); }
     void Remove(IntrusiveListNodeBase * node) { node->Remove(); }
 
+    /// @brief Replace an original node in list with a new node.
+    void Replace(IntrusiveListNodeBase * original, IntrusiveListNodeBase * replacement)
+    {
+        // VerifyOrDie(Contains(original)); This check is too heavy to do, but it shall hold
+        VerifyOrDie(!replacement->IsInList());
+        replacement->TakePlaceOf(original);
+        original->mPrev = nullptr;
+        original->mNext = nullptr;
+    }
+
     bool Contains(const IntrusiveListNodeBase * node) const
     {
         for (auto & iter : *this)
@@ -269,6 +318,9 @@ class IntrusiveList : public IntrusiveListBase
 
     IntrusiveList() : IntrusiveListBase() {}
 
+    IntrusiveList(IntrusiveList &&) = default;
+    IntrusiveList & operator=(IntrusiveList &&) = default;
+
     class ConstIterator : public IntrusiveListBase::ConstIteratorBase
     {
     public:
@@ -302,6 +354,7 @@ class IntrusiveList : public IntrusiveListBase
     void InsertBefore(Iterator pos, T * value) { IntrusiveListBase::InsertBefore(pos, Hook::ToNode(value)); }
     void InsertAfter(Iterator pos, T * value) { IntrusiveListBase::InsertAfter(pos, Hook::ToNode(value)); }
     void Remove(T * value) { IntrusiveListBase::Remove(Hook::ToNode(value)); }
+    void Replace(T * original, T * replacement) { IntrusiveListBase::Replace(Hook::ToNode(original), Hook::ToNode(replacement)); }
     bool Contains(const T * value) const { return IntrusiveListBase::Contains(Hook::ToNode(value)); }
 };
 

diff --git a/src/lib/support/tests/TestIntrusiveList.cpp b/src/lib/support/tests/TestIntrusiveList.cpp
@@ -131,6 +131,66 @@ void TestContains(nlTestSuite * inSuite, void * inContext)
     list.Remove(&b);
 }
 
+void TestReplaceNode(nlTestSuite * inSuite, void * inContext)
+{
+    ListNode a, b;
+    IntrusiveList<ListNode> list;
+    list.PushBack(&a);
+
+    list.Replace(&a, &b);
+    NL_TEST_ASSERT(inSuite, !a.IsInList());
+    NL_TEST_ASSERT(inSuite, b.IsInList());
+    NL_TEST_ASSERT(inSuite, !list.Empty());
+    NL_TEST_ASSERT(inSuite, !list.Contains(&a));
+    NL_TEST_ASSERT(inSuite, list.Contains(&b));
+    list.Remove(&b);
+}
+
+void TestMoveList(nlTestSuite * inSuite, void * inContext)
+{
+    ListNode a, b;
+
+    {
+        // Test case 1: Move construct an empty list
+        IntrusiveList<ListNode> listA;
+        IntrusiveList<ListNode> listB(std::move(listA));
+        NL_TEST_ASSERT(inSuite, listA.Empty()); // NOLINT(bugprone-use-after-move)
+        NL_TEST_ASSERT(inSuite, listB.Empty());
+    }
+
+    {
+        // Test case 2: Move construct an non-empty list
+        IntrusiveList<ListNode> listA;
+        listA.PushBack(&a);
+
+        IntrusiveList<ListNode> listB(std::move(listA));
+        NL_TEST_ASSERT(inSuite, listA.Empty()); // NOLINT(bugprone-use-after-move)
+        NL_TEST_ASSERT(inSuite, listB.Contains(&a));
+        listB.Remove(&a);
+    }
+
+    {
+        // Test case 3: Move assign an empty list
+        IntrusiveList<ListNode> listA;
+        IntrusiveList<ListNode> listB;
+        listB = std::move(listA);
+        NL_TEST_ASSERT(inSuite, listA.Empty()); // NOLINT(bugprone-use-after-move)
+        NL_TEST_ASSERT(inSuite, listB.Empty());
+    }
+
+    {
+        // Test case 4: Move assign to a non-empty list
+        IntrusiveList<ListNode> listA;
+        listA.PushBack(&a);
+
+        IntrusiveList<ListNode> listB;
+        listB = std::move(listA);
+        NL_TEST_ASSERT(inSuite, listA.Empty()); // NOLINT(bugprone-use-after-move)
+        NL_TEST_ASSERT(inSuite, listB.Contains(&a));
+        listB.Remove(&a);
+    }
+}
+
 int Setup(void * inContext)
 {
     return SUCCESS;
@@ -150,6 +210,8 @@ int Teardown(void * inContext)
 static const nlTest sTests[] = {
     NL_TEST_DEF_FN(TestIntrusiveListRandom), //
     NL_TEST_DEF_FN(TestContains),            //
+    NL_TEST_DEF_FN(TestReplaceNode),         //
+    NL_TEST_DEF_FN(TestMoveList),            //
     NL_TEST_SENTINEL(),                      //
 };