Fix AppendVector random access iterator (#917)

Author: Symmetricity

include/append_vector.h

@@ -5,6 +5,7 @@
 #include <vector>
 #include <queue>
 #include <cstdint>
+#include <iterator>
 
 // Tilemaker collects OutputObjects in a list that
 // - spills to disk
@@ -32,114 +33,111 @@ namespace AppendVectorNS {
 			using reference         = T&;
 
 			Iterator(AppendVector<T>& appendVector, uint16_t vec, uint16_t offset):
-				appendVector(&appendVector), vec(vec), offset(offset) {}
+				appendVector(&appendVector), index(difference_type(vec) * APPEND_VECTOR_SIZE + offset) {}
 
 			Iterator():
-				appendVector(nullptr), vec(0), offset(0) {}
+				appendVector(nullptr), index(0) {}
 
 
 			bool operator<(const Iterator& other) const {
-				if (vec < other.vec)
-					return true;
+				return index < other.index;
+			}
 
-				if (vec > other.vec)
-					return false;
+			bool operator>(const Iterator& other) const {
+				return other < *this;
+			}
 
-				return offset < other.offset;
+			bool operator<=(const Iterator& other) const {
+				return !(other < *this);
 			}
 
 			bool operator>=(const Iterator& other) const {
 				return !(*this < other);
 			}
 
-			Iterator operator-(int delta) const {
-				int64_t absolute = vec * APPEND_VECTOR_SIZE + offset;
-				absolute -= delta;
-				return Iterator(*appendVector, absolute / APPEND_VECTOR_SIZE, absolute % APPEND_VECTOR_SIZE);
+			Iterator operator-(difference_type delta) const {
+				Iterator result = *this;
+				result -= delta;
+				return result;
+			}
+
+			Iterator operator+(difference_type delta) const {
+				Iterator result = *this;
+				result += delta;
+				return result;
 			}
 
-			Iterator operator+(int delta) const {
-				int64_t absolute = vec * APPEND_VECTOR_SIZE + offset;
-				absolute += delta;
-				return Iterator(*appendVector, absolute / APPEND_VECTOR_SIZE, absolute % APPEND_VECTOR_SIZE);
+			friend Iterator operator+(difference_type delta, const Iterator& iterator) {
+				return iterator + delta;
 			}
 
 			bool operator==(const Iterator& other) const {
-				return appendVector == other.appendVector && vec == other.vec && offset == other.offset;
+				return appendVector == other.appendVector && index == other.index;
 			}
 
 			bool operator!=(const Iterator& other) const {
 				return !(*this == other);
 			}
 
-			std::ptrdiff_t operator-(const Iterator& other) const {
-				int64_t absolute = vec * APPEND_VECTOR_SIZE + offset;
-				int64_t otherAbsolute = other.vec * APPEND_VECTOR_SIZE + other.offset;
-
-				return absolute - otherAbsolute;
+			difference_type operator-(const Iterator& other) const {
+				return index - other.index;
 			}
 
 			reference operator*() const {
-				auto& vector = appendVector->vecs[vec];
-				auto& el = vector[offset];
+				auto& vector = appendVector->vecs[index / APPEND_VECTOR_SIZE];
+				auto& el = vector[index % APPEND_VECTOR_SIZE];
 				return el;
 			}
 
+			reference operator[](difference_type delta) const {
+				return *(*this + delta);
+			}
+
 			pointer operator->() const {
-				auto& vector = appendVector->vecs[vec];
-				auto& el = vector[offset];
+				auto& vector = appendVector->vecs[index / APPEND_VECTOR_SIZE];
+				auto& el = vector[index % APPEND_VECTOR_SIZE];
 				return &el;
 			}
 
-			Iterator& operator+= (int delta) {
-				int64_t absolute = vec * APPEND_VECTOR_SIZE + offset;
-				absolute += delta;
-
-				vec = absolute / APPEND_VECTOR_SIZE;
-				offset = absolute % APPEND_VECTOR_SIZE;
+			Iterator& operator+= (difference_type delta) {
+				index += delta;
 				return *this;
 			}
 
-			Iterator& operator-= (int delta) {
-				int64_t absolute = vec * APPEND_VECTOR_SIZE + offset;
-				absolute -= delta;
-
-				vec = absolute / APPEND_VECTOR_SIZE;
-				offset = absolute % APPEND_VECTOR_SIZE;
+			Iterator& operator-= (difference_type delta) {
+				index -= delta;
 				return *this;
 			}
 
 			// Prefix increment
 			Iterator& operator++() {
-				offset++;
-				if (offset == APPEND_VECTOR_SIZE) {
-					offset = 0;
-					vec++;
-				}
+				index++;
 				return *this;
 			}  
 
 			// Postfix increment
-			Iterator operator++(int) { Iterator tmp = *this; ++(*this); return tmp; }
+			Iterator operator++(int) {
+				Iterator result = *this;
+				++*this;
+				return result;
+			}
 
 			// Prefix decrement
 			Iterator& operator--() {
-				if (offset > 0) {
-					offset--;
-				} else {
-					vec--;
-					offset = APPEND_VECTOR_SIZE - 1;
-				}
-
+				index--;
 				return *this;
 			}
 
 			// Postfix decrement
-			Iterator operator--(int) { Iterator tmp = *this; --(*this); return tmp; }
+			Iterator operator--(int) {
+				Iterator result = *this;
+				--*this;
+				return result;
+			}
 
 		private:
 			mutable AppendVector<T>* appendVector;
-			int32_t vec, offset;
+			difference_type index;
 		};
 
 		AppendVector():

test/append_vector.test.cpp

@@ -1,4 +1,6 @@
 #include <iostream>
+#include <iterator>
+#include <type_traits>
 #include <boost/sort/sort.hpp>
 #include "external/minunit.h"
 #include "append_vector.h"
@@ -30,6 +32,32 @@ MU_TEST(test_append_vector) {
 	mu_check(*(vec.end() - 2) == 9998);
 	mu_check(*(vec.end() - 9000) == 1000);
 	mu_check(*(vec.begin() - -1) == 1);
+	mu_check(vec.begin()[25] == 25);
+	mu_check(25 + vec.begin() == vec.begin() + 25);
+	mu_check((vec.begin() + 25) - vec.begin() == 25);
+	mu_check(vec.end() - vec.begin() == 10000);
+	mu_check(vec.begin() < vec.end());
+	mu_check(vec.begin() <= vec.begin());
+	mu_check(vec.end() > vec.begin());
+	mu_check(vec.end() >= vec.end());
+
+	auto postfix = vec.begin();
+	mu_check(*(postfix++) == 0);
+	mu_check(*postfix == 1);
+	mu_check(*(postfix--) == 1);
+	mu_check(*postfix == 0);
+
+	const int32_t chunkBoundary = 8192;
+	auto boundary = vec.begin();
+	boundary += chunkBoundary;
+	mu_check(*boundary == chunkBoundary);
+	boundary -= 1;
+	mu_check(*boundary == chunkBoundary - 1);
+
+	static_assert(std::is_same<
+		std::iterator_traits<AppendVector<int32_t>::Iterator>::iterator_category,
+		std::random_access_iterator_tag
+	>::value, "AppendVector iterator should advertise random access");
 
 	boost::sort::block_indirect_sort(
 		vec.begin(),
@@ -95,4 +123,3 @@ int main() {
 	MU_REPORT();
 	return MU_EXIT_CODE;
 }
-