From 853cb8af5cb16080470b29f127b1d2664c4ca2c7 Mon Sep 17 00:00:00 2001 From: Lenny Maiorani Date: Thu, 24 Dec 2020 11:23:12 -0700 Subject: [PATCH] DoublyLinkedList: Implement `find` in terms of `AK::find` Problem: - The implementation of `find` is coupled to the implementation of `DoublyLinkedList`. - `append` and `prepend` are implemented multiple times so that r-value references can be moved from into the new node. This is probably not called very often because a pr-value or x-value needs to be used here. Solution: - Decouple the implementation of `find` from the class by using a generic `find` algorithm. - Make `append` and `prepend` be function templates so that they can have binding references which can be forwarded. --- AK/DoublyLinkedList.h | 130 +++++++++++------------------- AK/Tests/CMakeLists.txt | 1 + AK/Tests/TestDoublyLinkedList.cpp | 65 +++++++++++++++ Kernel/Devices/Device.h | 2 +- 4 files changed, 114 insertions(+), 84 deletions(-) create mode 100644 AK/Tests/TestDoublyLinkedList.cpp diff --git a/AK/DoublyLinkedList.h b/AK/DoublyLinkedList.h index 9990b9ce779..e679a3e37d5 100644 --- a/AK/DoublyLinkedList.h +++ b/AK/DoublyLinkedList.h @@ -27,6 +27,7 @@ #pragma once #include +#include #include #include @@ -60,13 +61,11 @@ template class DoublyLinkedList { private: struct Node { - explicit Node(const T& v) - : value(v) - { - } - explicit Node(T&& v) - : value(move(v)) + template + explicit Node(U&& v) + : value(forward(v)) { + static_assert(IsSame::value); } T value; Node* next { nullptr }; @@ -77,7 +76,7 @@ public: DoublyLinkedList() { } ~DoublyLinkedList() { clear(); } - bool is_empty() const { return !head(); } + bool is_empty() const { return !m_head; } void clear() { @@ -92,48 +91,64 @@ public: T& first() { - ASSERT(head()); - return head()->value; + ASSERT(m_head); + return m_head->value; } const T& first() const { - ASSERT(head()); - return head()->value; + ASSERT(m_head); + return m_head->value; } T& last() { - ASSERT(head()); - return tail()->value; + ASSERT(m_head); + return m_tail->value; } const T& last() const { - ASSERT(head()); - return tail()->value; + ASSERT(m_head); + return m_tail->value; } - void append(T&& value) + template + void append(U&& value) { - append_node(new Node(move(value))); + static_assert(IsSame::value); + auto* node = new Node(forward(value)); + if (!m_head) { + ASSERT(!m_tail); + m_head = node; + m_tail = node; + return; + } + ASSERT(m_tail); + ASSERT(!node->next); + m_tail->next = node; + node->prev = m_tail; + m_tail = node; } - void append(const T& value) + template + void prepend(U&& value) { - append_node(new Node(value)); - } - - void prepend(T&& value) - { - prepend_node(new Node(move(value))); - } - - void prepend(const T& value) - { - prepend_node(new Node(value)); + static_assert(IsSame::value); + auto* node = new Node(forward(value)); + if (!m_head) { + ASSERT(!m_tail); + m_head = node; + m_tail = node; + return; + } + ASSERT(m_tail); + ASSERT(!node->prev); + m_head->prev = node; + node->next = m_head; + m_head = node; } bool contains_slow(const T& value) const { - return find_node(value) != nullptr; + return find(value) != end(); } using Iterator = DoublyLinkedListIterator; @@ -148,18 +163,12 @@ public: ConstIterator find(const T& value) const { - Node* node = find_node(value); - if (node) - return ConstIterator(node); - return end(); + return AK::find(begin(), end(), value); } Iterator find(const T& value) { - Node* node = find_node(value); - if (node) - return Iterator(node); - return end(); + return AK::find(begin(), end(), value); } void remove(Iterator it) @@ -184,51 +193,6 @@ public: } private: - void append_node(Node* node) - { - if (!m_head) { - ASSERT(!m_tail); - m_head = node; - m_tail = node; - return; - } - ASSERT(m_tail); - ASSERT(!node->next); - m_tail->next = node; - node->prev = m_tail; - m_tail = node; - } - - void prepend_node(Node* node) - { - if (!m_head) { - ASSERT(!m_tail); - m_head = node; - m_tail = node; - return; - } - ASSERT(m_tail); - ASSERT(!node->prev); - m_head->prev = node; - node->next = m_head; - m_head = node; - } - - Node* find_node(const T& value) const - { - for (auto* node = m_head; node; node = node->next) { - if (Traits::equals(node->value, value)) - return node; - } - return nullptr; - } - - Node* head() { return m_head; } - const Node* head() const { return m_head; } - - Node* tail() { return m_tail; } - const Node* tail() const { return m_tail; } - Node* m_head { nullptr }; Node* m_tail { nullptr }; }; diff --git a/AK/Tests/CMakeLists.txt b/AK/Tests/CMakeLists.txt index 002b30092eb..4518b301403 100644 --- a/AK/Tests/CMakeLists.txt +++ b/AK/Tests/CMakeLists.txt @@ -11,6 +11,7 @@ set(AK_TEST_SOURCES TestCircularDuplexStream.cpp TestCircularQueue.cpp TestDistinctNumeric.cpp + TestDoublyLinkedList.cpp TestEndian.cpp TestFind.cpp TestFormat.cpp diff --git a/AK/Tests/TestDoublyLinkedList.cpp b/AK/Tests/TestDoublyLinkedList.cpp new file mode 100644 index 00000000000..06195bfadc5 --- /dev/null +++ b/AK/Tests/TestDoublyLinkedList.cpp @@ -0,0 +1,65 @@ +/* + * Copyright (c) 2021, the SerenityOS developers. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR + * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +#include + +#include + +static DoublyLinkedList make_list() +{ + DoublyLinkedList list {}; + list.append(0); + list.append(1); + list.append(2); + list.append(3); + list.append(4); + list.append(5); + list.append(6); + list.append(7); + list.append(8); + list.append(9); + return list; +} + +TEST_CASE(should_find_mutable) +{ + auto sut = make_list(); + + EXPECT_EQ(4, *sut.find(4)); + + EXPECT_EQ(sut.end(), sut.find(42)); +} + +TEST_CASE(should_find_const) +{ + const auto sut = make_list(); + + EXPECT_EQ(4, *sut.find(4)); + + EXPECT_EQ(sut.end(), sut.find(42)); +} + +TEST_MAIN(DoublyLinkedList) diff --git a/Kernel/Devices/Device.h b/Kernel/Devices/Device.h index 1ef8bcfa89e..b97ed35b254 100644 --- a/Kernel/Devices/Device.h +++ b/Kernel/Devices/Device.h @@ -75,7 +75,7 @@ public: { ScopedSpinLock lock(m_requests_lock); was_empty = m_requests.is_empty(); - m_requests.append(request); + m_requests.append(RefPtr(request)); } if (was_empty) request->do_start({});