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 <tim.jenssen@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>

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<typename Type,
          typename IndexType,
          typename Storage,
          typename Mutex,
-         typename Compare,
+         bool (*compare)(Utils::SmallStringView, Utils::SmallStringView),
-         Compare compare = Utils::compare,
+         class CacheEntry = StorageCacheEntry<Type, ViewType, IndexType>>
-         typename CacheEntry = StorageCacheEntry<Type, ViewType, IndexType>>
 class StorageCache
 {
     friend StorageCache;
@@ -78,7 +76,6 @@ public:
     using MutexType = Mutex;
     using CacheEntries = std::vector<CacheEntry>;
     using const_iterator = typename CacheEntries::const_iterator;
-    using Found = QmlDesigner::Found<const_iterator>;
 
     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<ViewType> &&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<Mutex> sharedLock(m_mutex);
 
-        Found found = find(view);
+        auto found = find(view);
 
-        if (found.wasFound)
+        if (found != m_entries.end())
-            return found.iterator->id;
+            return found->id;
 
         sharedLock.unlock();
         std::lock_guard<Mutex> exclusiveLock(m_mutex);
 
         if (!std::is_base_of<NonLockingMutex, Mutex>::value)
             found = find(view);
-        if (!found.wasFound) {
+        if (found == m_entries.end())
-            IndexType index = insertEntry(found.iterator, view, m_storage.fetchId(view));
+            found = insertEntry(found, view, m_storage.fetchId(view));
-            found.iterator = m_entries.begin() + index;
-        }
 
-        return found.iterator->id;
+        return found->id;
     }
 
     template<typename Container>
@@ -244,12 +239,11 @@ public:
 
         sharedLock.unlock();
         std::lock_guard<Mutex> 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<ResultType> values(const std::vector<IndexType> &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()

src/plugins/qmldesigner/designercore/metainfo/storagecachealgorithms.h

@@ -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 <utils/smallstringio.h>
-
-#include <iterator>
-
-#include <QDebug>
-
-namespace QmlDesigner {
-
-template <typename Iterator>
-class Found
-{
-public:
-    Iterator iterator;
-    bool wasFound;
-};
-
-template<typename ForwardIterator,
-         typename Type,
-         typename Compare>
-Found<ForwardIterator> findInSorted(ForwardIterator first, ForwardIterator last, const Type& value, Compare compare)
-{
-    ForwardIterator current;
-    using DifferenceType = typename std::iterator_traits<ForwardIterator>::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

src/plugins/qmldesigner/designercore/metainfo/storagecachefwd.h

@@ -31,6 +31,12 @@ namespace QmlDesigner {
 
 class NonLockingMutex;
 
-template<typename Type, typename ViewType, typename IndexType, typename Storage, typename Mutex, typename Compare, Compare compare, typename CacheEntry>
+template<typename Type,
+         typename ViewType,
+         typename IndexType,
+         typename Storage,
+         typename Mutex,
+         bool (*compare)(Utils::SmallStringView, Utils::SmallStringView),
+         typename CacheEntry>
 class StorageCache;
 } // namespace QmlDesigner

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 \

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<Utils::PathString,
+auto less(Utils::SmallStringView first, Utils::SmallStringView second) -> bool
-                                                       Utils::SmallStringView,
+{
-                                                       int,
+    return Utils::reverseCompare(first, second) < 0;
-                                                       StorageAdapter,
+};
-                                                       NiceMock<MockMutex>,
-                                                       decltype(&Utils::reverseCompare),
-                                                       Utils::reverseCompare>;
 
-using CacheWithoutLocking = QmlDesigner::StorageCache<Utils::PathString,
+using CacheWithMockLocking = QmlDesigner::
-                                                      Utils::SmallStringView,
+    StorageCache<Utils::PathString, Utils::SmallStringView, int, StorageAdapter, NiceMock<MockMutex>, less>;
-                                                      int,
+
-                                                      StorageAdapter,
+using CacheWithoutLocking = QmlDesigner::
-                                                      NiceMock<MockMutexNonLocking>,
+    StorageCache<Utils::PathString, Utils::SmallStringView, int, StorageAdapter, NiceMock<MockMutexNonLocking>, less>;
-                                                      decltype(&Utils::reverseCompare),
-                                                      Utils::reverseCompare>;
 
 template<typename Cache>
 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;