From c4c3d4cc3349a387d6ef85f856ebdd5b283bd668 Mon Sep 17 00:00:00 2001 From: Marco Bubke Date: Tue, 27 Apr 2021 10:31:29 +0200 Subject: [PATCH] QmlDesigner: Remove findInSorted function Using std::lower_bound is more generic and we have less code which could lead to less bugs. Change-Id: Iacf6ea550182b51a8d5dd6b18a92de68914f4df1 Reviewed-by: Tim Jenssen Reviewed-by: Qt CI Bot --- .../designercore/metainfo/storagecache.h | 46 ++++--- .../metainfo/storagecachealgorithms.h | 72 ---------- .../designercore/metainfo/storagecachefwd.h | 8 +- .../qmldesigner/qmldesignerunittestfiles.pri | 1 - tests/unit/unittest/storagecache-test.cpp | 129 ++---------------- 5 files changed, 40 insertions(+), 216 deletions(-) delete mode 100644 src/plugins/qmldesigner/designercore/metainfo/storagecachealgorithms.h diff --git a/src/plugins/qmldesigner/designercore/metainfo/storagecache.h b/src/plugins/qmldesigner/designercore/metainfo/storagecache.h index 88462e7ce50..55759c69f5d 100644 --- a/src/plugins/qmldesigner/designercore/metainfo/storagecache.h +++ b/src/plugins/qmldesigner/designercore/metainfo/storagecache.h @@ -25,7 +25,6 @@ #pragma once -#include "storagecachealgorithms.h" #include "storagecacheentry.h" #include "storagecachefwd.h" @@ -65,9 +64,8 @@ template> + bool (*compare)(Utils::SmallStringView, Utils::SmallStringView), + class CacheEntry = StorageCacheEntry> class StorageCache { friend StorageCache; @@ -78,7 +76,6 @@ public: using MutexType = Mutex; using CacheEntries = std::vector; using const_iterator = typename CacheEntries::const_iterator; - using Found = QmlDesigner::Found; StorageCache(Storage storage, std::size_t reserveSize = 1024) : m_storage{std::move(storage)} @@ -125,7 +122,7 @@ public: void uncheckedPopulate(CacheEntries &&entries) { std::sort(entries.begin(), entries.end(), [](ViewType first, ViewType second) { - return compare(first, second) < 0; + return compare(first, second); }); m_entries = std::move(entries); @@ -148,7 +145,7 @@ public: void add(std::vector &&views) { - auto less = [](ViewType first, ViewType second) { return compare(first, second) < 0; }; + auto less = [](ViewType first, ViewType second) { return compare(first, second); }; std::sort(views.begin(), views.end(), less); @@ -199,22 +196,20 @@ public: { std::shared_lock sharedLock(m_mutex); - Found found = find(view); + auto found = find(view); - if (found.wasFound) - return found.iterator->id; + if (found != m_entries.end()) + return found->id; sharedLock.unlock(); std::lock_guard exclusiveLock(m_mutex); if (!std::is_base_of::value) found = find(view); - if (!found.wasFound) { - IndexType index = insertEntry(found.iterator, view, m_storage.fetchId(view)); - found.iterator = m_entries.begin() + index; - } + if (found == m_entries.end()) + found = insertEntry(found, view, m_storage.fetchId(view)); - return found.iterator->id; + return found->id; } template @@ -244,12 +239,11 @@ public: sharedLock.unlock(); std::lock_guard exclusiveLock(m_mutex); - IndexType index; Type value{m_storage.fetchValue(id)}; - index = insertEntry(find(value).iterator, value, id); + auto interator = insertEntry(find(value), value, id); - return std::move(m_entries[index].value); + return interator->value; } std::vector values(const std::vector &ids) const @@ -280,9 +274,17 @@ private: m_indices[current->id] = std::distance(begin, current); } - Found find(ViewType view) + auto find(ViewType view) { - return findInSorted(m_entries.cbegin(), m_entries.cend(), view, compare); + auto found = std::lower_bound(m_entries.begin(), m_entries.end(), view, compare); + + if (found == m_entries.end()) + return m_entries.end(); + + if (*found == view) + return found; + + return m_entries.end(); } void incrementLargerOrEqualIndicesByOne(IndexType newIndex) @@ -301,7 +303,7 @@ private: m_indices.resize(id + 1, -1); } - IndexType insertEntry(const_iterator beforeIterator, ViewType view, IndexType id) + auto insertEntry(const_iterator beforeIterator, ViewType view, IndexType id) { auto inserted = m_entries.emplace(beforeIterator, view, id); @@ -312,7 +314,7 @@ private: ensureSize(id); m_indices.at(id) = newIndex; - return newIndex; + return inserted; } void checkEntries() diff --git a/src/plugins/qmldesigner/designercore/metainfo/storagecachealgorithms.h b/src/plugins/qmldesigner/designercore/metainfo/storagecachealgorithms.h deleted file mode 100644 index 42303ca3891..00000000000 --- a/src/plugins/qmldesigner/designercore/metainfo/storagecachealgorithms.h +++ /dev/null @@ -1,72 +0,0 @@ -/**************************************************************************** -** -** Copyright (C) 2021 The Qt Company Ltd. -** Contact: https://www.qt.io/licensing/ -** -** This file is part of Qt Creator. -** -** Commercial License Usage -** Licensees holding valid commercial Qt licenses may use this file in -** accordance with the commercial license agreement provided with the -** Software or, alternatively, in accordance with the terms contained in -** a written agreement between you and The Qt Company. For licensing terms -** and conditions see https://www.qt.io/terms-conditions. For further -** information use the contact form at https://www.qt.io/contact-us. -** -** GNU General Public License Usage -** Alternatively, this file may be used under the terms of the GNU -** General Public License version 3 as published by the Free Software -** Foundation with exceptions as appearing in the file LICENSE.GPL3-EXCEPT -** included in the packaging of this file. Please review the following -** information to ensure the GNU General Public License requirements will -** be met: https://www.gnu.org/licenses/gpl-3.0.html. -** -****************************************************************************/ - -#pragma once - -#include - -#include - -#include - -namespace QmlDesigner { - -template -class Found -{ -public: - Iterator iterator; - bool wasFound; -}; - -template -Found findInSorted(ForwardIterator first, ForwardIterator last, const Type& value, Compare compare) -{ - ForwardIterator current; - using DifferenceType = typename std::iterator_traits::difference_type; - DifferenceType count{std::distance(first, last)}; - DifferenceType step; - - while (count > 0) { - current = first; - step = count / 2; - std::advance(current, step); - auto comparison = compare(*current, value); - if (comparison < 0) { - first = ++current; - count -= step + 1; - } else if (comparison > 0) { - count = step; - } else { - return {current, true}; - } - } - - return {first, false}; -} - -} // namespace QmlDesigner diff --git a/src/plugins/qmldesigner/designercore/metainfo/storagecachefwd.h b/src/plugins/qmldesigner/designercore/metainfo/storagecachefwd.h index b949cc2d19f..3c5df91391c 100644 --- a/src/plugins/qmldesigner/designercore/metainfo/storagecachefwd.h +++ b/src/plugins/qmldesigner/designercore/metainfo/storagecachefwd.h @@ -31,6 +31,12 @@ namespace QmlDesigner { class NonLockingMutex; -template +template class StorageCache; } // namespace QmlDesigner diff --git a/src/plugins/qmldesigner/qmldesignerunittestfiles.pri b/src/plugins/qmldesigner/qmldesignerunittestfiles.pri index 161006d83c4..e8a4eaf3586 100644 --- a/src/plugins/qmldesigner/qmldesignerunittestfiles.pri +++ b/src/plugins/qmldesigner/qmldesignerunittestfiles.pri @@ -54,7 +54,6 @@ HEADERS += \ $$PWD/designercore/include/abstractproperty.h \ $$PWD/designercore/include/abstractview.h \ $$PWD/designercore/metainfo/storagecache.h \ - $$PWD/designercore/metainfo/storagecachealgorithms.h \ $$PWD/designercore/metainfo/storagecacheentry.h \ $$PWD/designercore/metainfo/storagecachefwd.h \ $$PWD/designercore/model/model_p.h \ diff --git a/tests/unit/unittest/storagecache-test.cpp b/tests/unit/unittest/storagecache-test.cpp index d68f3f07d0d..4ffb5caa8f1 100644 --- a/tests/unit/unittest/storagecache-test.cpp +++ b/tests/unit/unittest/storagecache-test.cpp @@ -39,7 +39,6 @@ using QmlDesigner::StorageCacheException; using uint64 = unsigned long long; -using QmlDesigner::findInSorted; using Utils::compare; using Utils::reverseCompare; @@ -53,21 +52,16 @@ public: MockFilePathStorage &storage; }; -using CacheWithMockLocking = QmlDesigner::StorageCache, - decltype(&Utils::reverseCompare), - Utils::reverseCompare>; +auto less(Utils::SmallStringView first, Utils::SmallStringView second) -> bool +{ + return Utils::reverseCompare(first, second) < 0; +}; -using CacheWithoutLocking = QmlDesigner::StorageCache, - decltype(&Utils::reverseCompare), - Utils::reverseCompare>; +using CacheWithMockLocking = QmlDesigner:: + StorageCache, less>; + +using CacheWithoutLocking = QmlDesigner:: + StorageCache, less>; template class StorageCache : public testing::Test @@ -275,111 +269,6 @@ TYPED_TEST(StorageCache, MultipleEntries) ASSERT_THROW(this->cache.populate(std::move(entries)), StorageCacheException); } -TYPED_TEST(StorageCache, DontFindInSorted) -{ - auto found = findInSorted(this->filePaths.cbegin(), this->filePaths.cend(), "/file/pathFoo", compare); - - ASSERT_FALSE(found.wasFound); -} - -TYPED_TEST(StorageCache, FindInSortedOne) -{ - auto found = findInSorted(this->filePaths.cbegin(), this->filePaths.cend(), "/file/pathOne", compare); - - ASSERT_TRUE(found.wasFound); -} - -TYPED_TEST(StorageCache, FindInSortedTwo) -{ - auto found = findInSorted(this->filePaths.cbegin(), this->filePaths.cend(), "/file/pathTwo", compare); - - ASSERT_TRUE(found.wasFound); -} - -TYPED_TEST(StorageCache, FindInSortedTree) -{ - auto found = findInSorted(this->filePaths.cbegin(), - this->filePaths.cend(), - "/file/pathThree", - compare); - - ASSERT_TRUE(found.wasFound); -} - -TYPED_TEST(StorageCache, FindInSortedFour) -{ - auto found = findInSorted(this->filePaths.cbegin(), this->filePaths.cend(), "/file/pathFour", compare); - - ASSERT_TRUE(found.wasFound); -} - -TYPED_TEST(StorageCache, FindInSortedFife) -{ - auto found = findInSorted(this->filePaths.cbegin(), this->filePaths.cend(), "/file/pathFife", compare); - - ASSERT_TRUE(found.wasFound); -} - -TYPED_TEST(StorageCache, DontFindInSortedReverse) -{ - auto found = findInSorted(this->reverseFilePaths.cbegin(), - this->reverseFilePaths.cend(), - "/file/pathFoo", - reverseCompare); - - ASSERT_FALSE(found.wasFound); -} - -TYPED_TEST(StorageCache, FindInSortedOneReverse) -{ - auto found = findInSorted(this->reverseFilePaths.cbegin(), - this->reverseFilePaths.cend(), - "/file/pathOne", - reverseCompare); - - ASSERT_TRUE(found.wasFound); -} - -TYPED_TEST(StorageCache, FindInSortedTwoReverse) -{ - auto found = findInSorted(this->reverseFilePaths.cbegin(), - this->reverseFilePaths.cend(), - "/file/pathTwo", - reverseCompare); - - ASSERT_TRUE(found.wasFound); -} - -TYPED_TEST(StorageCache, FindInSortedTreeReverse) -{ - auto found = findInSorted(this->reverseFilePaths.cbegin(), - this->reverseFilePaths.cend(), - "/file/pathThree", - reverseCompare); - - ASSERT_TRUE(found.wasFound); -} - -TYPED_TEST(StorageCache, FindInSortedFourReverse) -{ - auto found = findInSorted(this->reverseFilePaths.cbegin(), - this->reverseFilePaths.cend(), - "/file/pathFour", - reverseCompare); - - ASSERT_TRUE(found.wasFound); -} - -TYPED_TEST(StorageCache, FindInSortedFifeReverse) -{ - auto found = findInSorted(this->reverseFilePaths.cbegin(), - this->reverseFilePaths.cend(), - "/file/pathFife", - reverseCompare); - - ASSERT_TRUE(found.wasFound); -} - TYPED_TEST(StorageCache, IdIsReadAndWriteLockedForUnknownEntry) { InSequence s;