2665: Ensure that StepIterator does not advance past end of sequence. (…

…TrenchBroom#2667) * 2665: Ensure that StepIterator does not advance past end of sequence. * 2665: Add test for step iterator.
GoaLitiuM · Mar 22, 2019 · fb4a2b7 · fb4a2b7
1 parent 3e6e12a
commit fb4a2b7
Show file tree

Hide file tree

Showing 4 changed files with 101 additions and 29 deletions.
diff --git a/common/src/StepIterator.h b/common/src/StepIterator.h
@@ -31,45 +31,57 @@ class StepIterator {
     using pointer = typename I::pointer;
     using reference = typename I::reference;
 private:
-    I m_delegate;
+    I m_cur;
+    I m_end;
     difference_type m_stride;
-    bool m_initialStep;
 public:
-    StepIterator(I delegate, const difference_type offset = 0, const difference_type stride = 1) :
-    m_delegate(delegate),
+    StepIterator(I cur, I end, const difference_type offset = 0, const difference_type stride = 1) :
+    m_cur(cur),
+    m_end(end),
     m_stride(stride) {
-        std::advance(m_delegate, offset);
+        increment(offset);
     }
 
-    bool operator<(const StepIterator& other) const  { return m_delegate < other.m_delegate; }
-    bool operator>(const StepIterator& other) const  { return m_delegate > other.m_delegate; }
-    bool operator==(const StepIterator& other) const { return m_delegate == other.m_delegate; }
-    bool operator!=(const StepIterator& other) const { return m_delegate != other.m_delegate; }
+    friend bool operator<(const StepIterator& lhs, const StepIterator& rhs) { return lhs.m_cur < rhs.m_cur; }
+    friend bool operator<(const StepIterator& lhs, const I& rhs)            { return lhs.m_cur < rhs;       }
+    friend bool operator<(const I& lhs, const StepIterator& rhs)            { return lhs       < rhs.m_cur; }
+
+    friend bool operator>(const StepIterator& lhs, const StepIterator& rhs) { return lhs.m_cur > rhs.m_cur; }
+    friend bool operator>(const StepIterator& lhs, const I& rhs)            { return lhs.m_cur > rhs;       }
+    friend bool operator>(const I& lhs, const StepIterator& rhs)            { return lhs       > rhs.m_cur; }
+
+    friend bool operator==(const StepIterator& lhs, const StepIterator& rhs) { return lhs.m_cur == rhs.m_cur; }
+    friend bool operator==(const StepIterator& lhs, const I& rhs)            { return lhs.m_cur == rhs;       }
+    friend bool operator==(const I& lhs, const StepIterator& rhs)            { return lhs       == rhs.m_cur; }
+
+    friend bool operator!=(const StepIterator& lhs, const StepIterator& rhs) { return lhs.m_cur != rhs.m_cur; }
+    friend bool operator!=(const StepIterator& lhs, const I& rhs)            { return lhs.m_cur != rhs;       }
+    friend bool operator!=(const I& lhs, const StepIterator& rhs)            { return lhs       != rhs.m_cur; }
 
     // prefix increment
     StepIterator& operator++() {
-        increment();
+        increment(m_stride);
         return *this;
     }
 
     // postfix increment
     StepIterator operator++(int) {
         auto result = StepIterator(*this);
-        increment();
+        increment(m_stride);
         return result;
     }
 
-    reference operator*() const { return *m_delegate; }
-    pointer operator->() const { return *m_delegate; }
+    reference operator*() const { return *m_cur; }
+    pointer operator->() const { return *m_cur; }
 private:
-    void increment() {
-        std::advance(m_delegate, m_stride);
+    void increment(const typename I::difference_type distance) {
+        std::advance(m_cur, std::min(distance, m_end - m_cur));
     }
 };
 
 template <typename I>
-StepIterator<I> stepIterator(I delegate, const typename I::difference_type offset = 0, const typename I::difference_type stride = 1) {
-    return StepIterator<I>(delegate, offset, stride);
+StepIterator<I> stepIterator(I cur, I end, const typename I::difference_type offset = 0, const typename I::difference_type stride = 1) {
+    return StepIterator<I>(cur, end, offset, stride);
 }
 
 #endif //TRENCHBROOM_STEPITERATOR_H
diff --git a/common/src/View/EntityBrowserView.cpp b/common/src/View/EntityBrowserView.cpp
@@ -407,9 +407,9 @@ namespace TrenchBroom {
                         const auto quads = font.quads(title, false, offset);
                         const auto titleVertices = TextVertex::toList(
                             quads.size() / 2,
-                            stepIterator(std::begin(quads), 0, 2),
-                            stepIterator(std::begin(quads), 1, 2),
-                            stepIterator(std::begin(textColor), 0, 0));
+                            stepIterator(std::begin(quads), std::end(quads), 0, 2),
+                            stepIterator(std::begin(quads), std::end(quads), 1, 2),
+                            stepIterator(std::begin(textColor), std::end(textColor), 0, 0));
                         VectorUtils::append(stringVertices[defaultDescriptor], titleVertices);
                     }
 
@@ -425,9 +425,9 @@ namespace TrenchBroom {
                                 const auto quads = font.quads(cell.item().entityDefinition->name(), false, offset);
                                 const auto titleVertices = TextVertex::toList(
                                     quads.size() / 2,
-                                    stepIterator(std::begin(quads), 0, 2),
-                                    stepIterator(std::begin(quads), 1, 2),
-                                    stepIterator(std::begin(textColor), 0, 0));
+                                    stepIterator(std::begin(quads), std::end(quads), 0, 2),
+                                    stepIterator(std::begin(quads), std::end(quads), 1, 2),
+                                    stepIterator(std::begin(textColor), std::end(textColor), 0, 0));
                                 VectorUtils::append(stringVertices[cell.item().fontDescriptor], titleVertices);
                             }
                         }

diff --git a/common/src/View/TextureBrowserView.cpp b/common/src/View/TextureBrowserView.cpp
@@ -427,9 +427,9 @@ namespace TrenchBroom {
                         const auto quads = font.quads(title, false, offset);
                         const auto titleVertices = TextVertex::toList(
                             quads.size() / 2,
-                            stepIterator(std::begin(quads), 0, 2),
-                            stepIterator(std::begin(quads), 1, 2),
-                            stepIterator(std::begin(textColor), 0, 0));
+                            stepIterator(std::begin(quads), std::end(quads), 0, 2),
+                            stepIterator(std::begin(quads), std::end(quads), 1, 2),
+                            stepIterator(std::begin(textColor), std::end(textColor), 0, 0));
                         auto& vertices = stringVertices[defaultDescriptor];
                         vertices.insert(std::end(vertices), std::begin(titleVertices), std::end(titleVertices));
                     }
@@ -446,9 +446,9 @@ namespace TrenchBroom {
                                 const auto quads = font.quads(cell.item().texture->name(), false, offset);
                                 const auto titleVertices = TextVertex::toList(
                                     quads.size() / 2,
-                                    stepIterator(std::begin(quads), 0, 2),
-                                    stepIterator(std::begin(quads), 1, 2),
-                                    stepIterator(std::begin(textColor), 0, 0));
+                                    stepIterator(std::begin(quads), std::end(quads), 0, 2),
+                                    stepIterator(std::begin(quads), std::end(quads), 1, 2),
+                                    stepIterator(std::begin(textColor), std::end(textColor), 0, 0));
                                 auto& vertices = stringVertices[cell.item().fontDescriptor];
                                 vertices.insert(std::end(vertices), std::begin(titleVertices), std::end(titleVertices));
                             }

diff --git a/test/src/StepIteratorTest.cpp b/test/src/StepIteratorTest.cpp
@@ -0,0 +1,60 @@
+/*
+ Copyright (C) 2010-2019 Kristian Duske
+
+ This file is part of TrenchBroom.
+
+ TrenchBroom is free software: you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation, either version 3 of the License, or
+ (at your option) any later version.
+
+ TrenchBroom is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with TrenchBroom. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <gtest/gtest.h>
+
+#include "StepIterator.h"
+
+#include <vector>
+
+TEST(StepIteratorTest, emptySequence) {
+    std::vector<size_t> vec;
+
+    ASSERT_EQ(std::begin(vec), stepIterator(std::begin(vec), std::end(vec)));
+    ASSERT_EQ(std::end(vec), stepIterator(std::begin(vec), std::end(vec)));
+    ASSERT_EQ(std::begin(vec), stepIterator(std::begin(vec), std::end(vec), 1));
+    ASSERT_EQ(std::end(vec), stepIterator(std::begin(vec), std::end(vec), 1));
+}
+
+TEST(StepIteratorTest, zeroStride) {
+    std::vector<size_t> vec({ 1 });
+
+    ASSERT_EQ(std::begin(vec), stepIterator(std::begin(vec), std::end(vec), 0, 0));
+    ASSERT_EQ(std::begin(vec), std::next(stepIterator(std::begin(vec), std::end(vec), 0, 0)));
+}
+
+TEST(StepIteratorTest, oneElementSequence) {
+    std::vector<size_t> vec({ 1 });
+
+    ASSERT_EQ(std::begin(vec), stepIterator(std::begin(vec), std::end(vec)));
+    ASSERT_EQ(std::end(vec), stepIterator(std::begin(vec), std::end(vec), 1));
+    ASSERT_EQ(std::end(vec), stepIterator(std::begin(vec), std::end(vec), 2));
+
+    ASSERT_EQ(std::begin(vec), stepIterator(std::begin(vec), std::end(vec), 0, 2));
+    ASSERT_EQ(std::end(vec), stepIterator(std::begin(vec), std::end(vec), 1, 2));
+    ASSERT_EQ(std::end(vec), stepIterator(std::begin(vec), std::end(vec), 2, 2));
+
+    ASSERT_EQ(std::end(vec), std::next(stepIterator(std::begin(vec), std::end(vec), 0)));
+    ASSERT_EQ(std::end(vec), std::next(stepIterator(std::begin(vec), std::end(vec), 1)));
+    ASSERT_EQ(std::end(vec), std::next(stepIterator(std::begin(vec), std::end(vec), 2)));
+
+    ASSERT_EQ(std::end(vec), std::next(stepIterator(std::begin(vec), std::end(vec), 0, 2)));
+    ASSERT_EQ(std::end(vec), std::next(stepIterator(std::begin(vec), std::end(vec), 1, 2)));
+    ASSERT_EQ(std::end(vec), std::next(stepIterator(std::begin(vec), std::end(vec), 2, 2)));
+}