fix: potential undefined behaviour in nth (#104)

Author: uncommon-nickname

include/rusty_iterators/interface.hpp

@@ -64,7 +64,7 @@ class IterInterface
     IterInterface(IterInterface&&)                 = default;
     IterInterface& operator=(IterInterface&&)      = default;
 
-    [[nodiscard]] auto advanceBy(size_t amount) -> Derived;
+    auto advanceBy(size_t amount) -> void;
 
     template <class Functor>
         requires AnyFunctor<T, Functor>
@@ -183,14 +183,13 @@ class IterInterface
 } // namespace rusty_iterators::interface
 
 template <class T, class Derived>
-auto rusty_iterators::interface::IterInterface<T, Derived>::advanceBy(size_t amount) -> Derived
+auto rusty_iterators::interface::IterInterface<T, Derived>::advanceBy(size_t n) -> void
 {
-    for (size_t i = 0; i < amount; i++)
+    for (size_t i = 0; i < n; ++i)
     {
         [[unlikely]] if (!self().next().has_value())
             break;
     }
-    return std::move(self());
 }
 
 template <class T, class Derived>
@@ -222,8 +221,7 @@ auto rusty_iterators::interface::IterInterface<T, Derived>::collect() -> std::ve
     auto size       = sizeHintChecked();
 
     collection.reserve(size);
-
-    forEach([&collection](auto&& x) { collection.push_back(std::move(x)); });
+    self().forEach([&collection](auto&& x) { collection.push_back(std::move(x)); });
 
     return std::move(collection);
 }
@@ -233,7 +231,7 @@ auto rusty_iterators::interface::IterInterface<T, Derived>::count() -> size_t
 {
     size_t count = 0;
 
-    forEach([&count](auto&& _) { count += 1; });
+    self().forEach([&count](auto&& _) { count += 1; });
 
     return count;
 }
@@ -404,9 +402,10 @@ auto rusty_iterators::interface::IterInterface<T, Derived>::neBy(Other&& it, Fun
 }
 
 template <class T, class Derived>
-auto rusty_iterators::interface::IterInterface<T, Derived>::nth(size_t element) -> std::optional<T>
+auto rusty_iterators::interface::IterInterface<T, Derived>::nth(size_t n) -> std::optional<T>
 {
-    return self().advanceBy(element).next();
+    self().advanceBy(n);
+    return self().next();
 }
 
 template <class T, class Derived>

include/rusty_iterators/skip.hpp

@@ -29,9 +29,9 @@ auto rusty_iterators::iterator::Skip<T, Other>::next() -> std::optional<T>
 {
     [[unlikely]] if (n > 0)
     {
-        it = it.advanceBy(n);
-        n  = 0;
-        return std::move(it.next());
+        auto item = it.nth(n);
+        n         = 0;
+        return std::move(item);
     }
     return std::move(it.next());
 }

include/rusty_iterators/zip.hpp

@@ -18,7 +18,7 @@ class Zip : public IterInterface<std::tuple<T, R>, Zip<T, R, First, Second>>
         : first(std::forward<First>(f)), second(std::forward<Second>(s))
     {}
 
-    [[nodiscard]] auto advanceBy(size_t amount) -> Zip<T, R, First, Second>;
+    auto advanceBy(size_t amount) -> void;
     auto next() -> std::optional<std::tuple<T, R>>;
     [[nodiscard]] auto sizeHint() const -> std::optional<size_t>;
 
@@ -29,13 +29,10 @@ class Zip : public IterInterface<std::tuple<T, R>, Zip<T, R, First, Second>>
 } // namespace rusty_iterators::iterator
 
 template <class T, class R, class First, class Second>
-auto rusty_iterators::iterator::Zip<T, R, First, Second>::advanceBy(size_t amount)
-    -> Zip<T, R, First, Second>
+auto rusty_iterators::iterator::Zip<T, R, First, Second>::advanceBy(size_t amount) -> void
 {
-    first  = first.advanceBy(amount);
-    second = second.advanceBy(amount);
-
-    return std::move(static_cast<Zip<T, R, First, Second>&>(*this));
+    first.advanceBy(amount);
+    second.advanceBy(amount);
 }
 
 template <class T, class R, class First, class Second>

tests/file_iterator.test.cpp

@@ -81,7 +81,9 @@ TEST(TestBufferedFileIterator, TestSizeHint)
 TEST(TestBufferedFileIterator, TestSizeHintAfterConsumption)
 {
     auto testFileName = std::string{"./tests/test_data.txt"};
-    auto it           = FileIterator<FIterType::Buffered>{testFileName}.advanceBy(2);
+    auto it           = FileIterator<FIterType::Buffered>{testFileName};
+
+    it.advanceBy(2);
 
     ASSERT_EQ(it.sizeHint(), 2);
 }

tests/integration.test.cpp

@@ -87,7 +87,9 @@ TEST(TestIteratorIntegration, TestWindowsOverCycle)
 TEST(TestIteratorIntegration, TestCycleAdvanceBy)
 {
     auto vec = std::vector{1, 2};
-    auto it  = LazyIterator{vec}.cycle().advanceBy(3);
+    auto it  = LazyIterator{vec}.cycle();
+
+    it.advanceBy(3);
 
     ASSERT_EQ(it.next().value(), 2);
     ASSERT_EQ(it.next().value(), 1);

tests/iterator.test.cpp

@@ -211,15 +211,19 @@ TEST(TestIterator, AllReturnsFalseIfOneDoesntFit)
 TEST(TestIterator, AdvanceByMovesIteratorPtr)
 {
     auto vec = std::vector{1, 2, 3, 4};
-    auto it  = LazyIterator{vec}.advanceBy(3);
+    auto it  = LazyIterator{vec};
+
+    it.advanceBy(3);
 
     ASSERT_EQ(it.next().value(), 4);
 }
 
 TEST(TestIterator, AdvanceByBiggerThanSize)
 {
     auto vec = std::vector{1, 2, 3};
-    auto it  = LazyIterator{vec}.advanceBy(4);
+    auto it  = LazyIterator{vec};
+
+    it.advanceBy(4);
 
     ASSERT_EQ(it.next(), std::nullopt);
 }

tests/zip.test.cpp

@@ -76,7 +76,9 @@ TEST(TestZipIterator, TestAdvanceBy)
     auto v1 = std::vector{1, 2, 3};
     auto v2 = std::vector{4, 5, 6};
 
-    auto it = LazyIterator{v1}.zip(LazyIterator{v2}).advanceBy(2);
+    auto it = LazyIterator{v1}.zip(LazyIterator{v2});
+
+    it.advanceBy(2);
 
     EXPECT_THAT(it.next().value(), FieldsAre(3, 6));
 }