rewrite search function to use iterators

Until now search() works on an array like object. This works fine if the
whole array is searched. But if only a part is searched, this creates an
unnecessary copy.

src/models/disassemblymodel.cpp

@@ -244,7 +244,8 @@ void DisassemblyModel::find(const QString& search, Direction direction, int curr
 
     auto endReached = [this] { emit searchEndReached(); };
 
-    const int resultIndex = ::search(m_data.disassemblyLines, current, direction, searchFunc, endReached);
+    const int resultIndex = ::search(m_data.disassemblyLines.cbegin(), m_data.disassemblyLines.cend(), current,
+                                     direction, searchFunc, endReached);
 
     if (resultIndex >= 0) {
         emit resultFound(createIndex(resultIndex, DisassemblyColumn));

src/models/search.h

@@ -9,27 +9,24 @@
 
 #include <algorithm>
 #include <iterator>
-#include <QVector>
 
 enum class Direction
 {
     Forward,
     Backward
 };
 
-/** a search function that wrap around at the end
+/** a search function that wraps around at the end
  * this function evaluates searchFunc starting from current and returns the offset from begin. In case end is reached,
  * it calls endReached and starts again at begin
  *
  * return: offset from begin
  * */
-template<typename iter, typename SearchFunc, typename EndReached>
-int search_impl(iter begin, iter end, iter current, SearchFunc searchFunc, EndReached endReached)
+template<typename it, typename SearchFunc, typename EndReached>
+int search_helper(const it begin, const it end, const it current, SearchFunc searchFunc, EndReached endReached)
 {
-    if (begin == end)
-        return -1;
-
-    const auto start = (current == end) ? begin : (current + 1);
+    // if current points to the last line, current will now point to end -> wrap around
+    const auto start = (current == end) ? begin : current;
     auto found = std::find_if(start, end, searchFunc);
 
     if (found != end) {
@@ -40,28 +37,28 @@ int search_impl(iter begin, iter end, iter current, SearchFunc searchFunc, EndRe
 
     found = std::find_if(begin, start, searchFunc);
 
-    if (found != end) {
+    if (found != start) {
         return std::distance(begin, found);
     }
 
     return -1;
 }
 
-// a wrapper around search_impl that returns the result index from begin
-template<typename Array, typename SearchFunc, typename EndReached>
-int search(const Array& array, int current, Direction direction, SearchFunc searchFunc, EndReached endReached)
+template<typename it, typename SearchFunc, typename EndReached>
+int search(const it begin, const it end, int current, Direction direction, SearchFunc searchFunc, EndReached endReached)
 {
-    current = std::clamp(0, static_cast<int>(array.size()), current);
-    int resultIndex = 0;
+    if (begin == end)
+        return -1;
+
+    const auto size = std::distance(begin, end);
+    current = std::clamp(current, 0, static_cast<int>(size) - 1);
+    const auto currentIt = begin + current;
 
     if (direction == Direction::Forward) {
-        resultIndex = ::search_impl(array.begin(), array.end(), array.begin() + current, searchFunc, endReached);
-    } else {
-        resultIndex = ::search_impl(array.rbegin(), array.rend(),
-                                    std::make_reverse_iterator(array.begin() + current) - 1, searchFunc, endReached);
-        if (resultIndex != -1) {
-            resultIndex = array.size() - resultIndex - 1;
-        }
+        return search_helper(begin, end, std::next(currentIt), searchFunc, endReached);
     }
-    return resultIndex;
+
+    int resultIndex = search_helper(std::make_reverse_iterator(end), std::make_reverse_iterator(begin),
+                                    std::make_reverse_iterator(currentIt), searchFunc, endReached);
+    return resultIndex != -1 ? (size - resultIndex - 1) : -1;
 }

src/models/sourcecodemodel.cpp

@@ -254,8 +254,8 @@ void SourceCodeModel::find(const QString& search, Direction direction, int curre
     auto searchFunc = [&search](const QString& line) { return line.indexOf(search, 0, Qt::CaseInsensitive) != -1; };
 
     auto endReached = [this] { emit searchEndReached(); };
-
-    const int resultIndex = ::search(m_lines.mid(m_startLine, m_numLines), current, direction, searchFunc, endReached);
+    const int resultIndex = ::search(m_lines.cbegin() + m_startLine, m_lines.cbegin() + m_startLine + m_numLines,
+                                     current, direction, searchFunc, endReached);
 
     if (resultIndex >= 0) {
         emit resultFound(createIndex(resultIndex + 1, SourceCodeColumn));

src/resultsdisassemblypage.cpp

@@ -402,7 +402,6 @@ ResultsDisassemblyPage::ResultsDisassemblyPage(CostContextMenu* costContextMenu,
 
         auto searchPrev = [model, edit, additionalRows, searchResultIndex] {
             const auto offset = searchResultIndex->isValid() ? searchResultIndex->row() - additionalRows : 0;
-
             model->find(edit->text(), Direction::Backward, offset);
         };
 

tests/modeltests/tst_models.cpp

@@ -11,10 +11,13 @@
 #include <QObject>
 #include <QProcess>
 #include <QRegularExpression>
+#include <QSignalSpy>
+#include <QTemporaryFile>
 #include <QTest>
 #include <QTextStream>
 
 #include "../testutils.h"
+#include "search.h"
 
 #include <models/disassemblymodel.h>
 #include <models/eventmodel.h>
@@ -473,6 +476,88 @@ private slots:
             28);
     }
 
+    void testSourceCodeModelSearch()
+    {
+        QTemporaryFile file;
+        if (!file.open()) {
+            QSKIP("Failed to create test file");
+        }
+
+        for (int i = 0; i < 10; i++) {
+            file.write(QStringLiteral("Line %1\n").arg(i).toUtf8());
+        }
+
+        file.flush();
+
+        DisassemblyOutput output;
+        output.mainSourceFileName = file.fileName();
+        output.realSourceFileName = file.fileName();
+
+        DisassemblyOutput::DisassemblyLine line1;
+        line1.fileLine = Data::FileLine {file.fileName(), 4};
+
+        DisassemblyOutput::DisassemblyLine line2;
+        line2.fileLine = Data::FileLine {file.fileName(), 8};
+
+        output.disassemblyLines = {line1, line2};
+
+        SourceCodeModel model(nullptr);
+        model.setDisassembly(output, {});
+
+        QCOMPARE(model.rowCount(), 6); // 5 lines + function name
+        QCOMPARE(model.data(model.index(1, SourceCodeModel::SourceCodeColumn), Qt::DisplayRole).value<QString>(),
+                 QStringLiteral("Line 3"));
+
+        QCOMPARE(model.data(model.index(5, SourceCodeModel::SourceCodeColumn), Qt::DisplayRole).value<QString>(),
+                 QStringLiteral("Line 7"));
+
+        // check if search works in general
+        QSignalSpy searchSpy(&model, &SourceCodeModel::resultFound);
+        for (int i = 0; i < 5; i++) {
+            model.find(QStringLiteral("Line 5"), Direction::Forward, i);
+            auto result = searchSpy.takeFirst();
+            QCOMPARE(result.at(0).value<QModelIndex>(), model.index(3, SourceCodeModel::SourceCodeColumn));
+        }
+
+        // Check wrap around
+        for (int i = 1; i < 4; i++) {
+            QSignalSpy endReached(&model, &SourceCodeModel::searchEndReached);
+            model.find(QStringLiteral("Line 3"), Direction::Forward, i);
+            QCOMPARE(endReached.size(), 1);
+        }
+
+        // check if no result found works
+        searchSpy.clear();
+        for (int i = 0; i < 5; i++) {
+            model.find(QStringLiteral("Line 8"), Direction::Forward, i);
+            auto result = searchSpy.takeFirst();
+            qDebug() << i;
+            QCOMPARE(result.at(0).value<QModelIndex>().isValid(), false);
+        }
+
+        // test backward search
+        for (int i = 4; i > 0; i--) {
+            model.find(QStringLiteral("Line 7"), Direction::Backward, i);
+            auto result = searchSpy.takeFirst();
+            QCOMPARE(result.at(0).value<QModelIndex>(), model.index(5, SourceCodeModel::SourceCodeColumn));
+        }
+
+        // Check wrap around
+        for (int i = 4; i > 0; i--) {
+            QSignalSpy endReached(&model, &SourceCodeModel::searchEndReached);
+            model.find(QStringLiteral("Line 7"), Direction::Backward, i);
+            QCOMPARE(endReached.size(), 1);
+        }
+
+        // check if no result found works
+        searchSpy.clear();
+        for (int i = 0; i < 5; i++) {
+            model.find(QStringLiteral("Line 8"), Direction::Backward, i);
+            auto result = searchSpy.takeFirst();
+            QCOMPARE(result.at(0).value<QModelIndex>().isValid(), false);
+        }
+    }
+
     void testEventModel()
     {
         Data::EventResults events;

tests/modeltests/tst_search.cpp

@@ -23,11 +23,11 @@ private slots:
     {
         const std::array<int, 0> testArray = {};
 
-        QCOMPARE(search_impl(
-                     testArray.begin(), testArray.end(), testArray.begin(), [](int) { return false; }, [] {}),
+        QCOMPARE(search(
+                     testArray.cbegin(), testArray.cend(), 0, Direction::Forward, [](int) { return false; }, [] {}),
                  -1);
-        QCOMPARE(search_impl(
-                     testArray.rbegin(), testArray.rend(), testArray.rbegin(), [](int) { return false; }, [] {}),
+        QCOMPARE(search(
+                     testArray.cbegin(), testArray.cend(), 0, Direction::Backward, [](int) { return false; }, [] {}),
                  -1);
     }
     void testSearch()
@@ -36,14 +36,14 @@ private slots:
 
         int maxOffset = testArray.size() - 1;
         for (int offset = 0; offset < maxOffset; offset++) {
-            QCOMPARE(search_impl(
-                         testArray.begin(), testArray.end(), testArray.begin() + offset,
+            QCOMPARE(search(
+                         testArray.cbegin(), testArray.cend(), offset, Direction::Forward,
                          [](int num) { return num == 2; }, [] {}),
                      1);
-            QCOMPARE(search_impl(
-                         testArray.rbegin(), testArray.rend(), testArray.rbegin() + offset,
+            QCOMPARE(search(
+                         testArray.cbegin(), testArray.cend(), offset, Direction::Backward,
                          [](int num) { return num == 2; }, [] {}),
-                     3);
+                     1);
         }
     }
 
@@ -52,35 +52,20 @@ private slots:
         const std::array<int, 5> testArray = {1, 2, 3, 4, 5};
         {
             bool endReached = false;
-            QCOMPARE(search_impl(
-                         testArray.begin(), testArray.end(), testArray.begin() + 1, [](int i) { return i == 1; },
+            QCOMPARE(search(
+                         testArray.cbegin(), testArray.cend(), 1, Direction::Forward, [](int i) { return i == 1; },
                          [&endReached] { endReached = true; }),
                      0);
             QCOMPARE(endReached, true);
         }
 
         {
             bool endReached = false;
-            QCOMPARE(search_impl(
-                         testArray.rbegin(), testArray.rend(), std::make_reverse_iterator(testArray.begin() + 4 - 1),
-                         [](int i) { return i == 4; }, [&endReached] { endReached = true; }),
-                     1);
-            QCOMPARE(endReached, true);
-        }
-    }
-
-    void testWrapper()
-    {
-        const std::array<int, 5> testArray = {1, 2, 3, 4, 5};
-
-        int maxOffset = testArray.size() - 1;
-        for (int i = 0; i < maxOffset; i++) {
-            QCOMPARE(search(
-                         testArray, i, Direction::Forward, [](int i) { return i == 4; }, [] {}),
-                     3);
             QCOMPARE(search(
-                         testArray, i, Direction::Backward, [](int i) { return i == 4; }, [] {}),
+                         testArray.cbegin(), testArray.cend(), 1, Direction::Backward, [](int i) { return i == 4; },
+                         [&endReached] { endReached = true; }),
                      3);
+            QCOMPARE(endReached, true);
         }
     }
 
@@ -90,7 +75,7 @@ private slots:
 
         for (int i = 0; i < 2; i++) {
             QCOMPARE(search(
-                         testArray, i, Direction::Forward, [](int) { return true; }, [] {}),
+                         testArray.cbegin(), testArray.cend(), 0, Direction::Forward, [](int) { return true; }, [] {}),
                      -1);
         }
     }
@@ -100,9 +85,21 @@ private slots:
         const std::array<int, 1> testArray = {0};
 
         QCOMPARE(search(
-                     testArray, 1, Direction::Forward, [](int i) { return i == 0; }, [] {}),
+                     testArray.cbegin(), testArray.cend(), 1, Direction::Forward, [](int i) { return i == 0; }, [] {}),
                  0);
     }
+
+    void testSearchOnIterators()
+    {
+        const std::array<int, 5> testArray = {0, 1, 2, 3, 0};
+
+        for (int i = 1; i < 3; i++) {
+            QCOMPARE(search(
+                         testArray.cbegin() + 1, testArray.cbegin() + 3, i, Direction::Forward,
+                         [](int i) { return i == 0; }, [] {}),
+                     -1);
+        }
+    }
 };
 
 QTEST_GUILESS_MAIN(TestSearch)