Custom C++ random access iterator for pointers to arrays

The name of the pictureThe name of the pictureThe name of the pictureClash Royale CLAN TAG#URR8PPP





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;







up vote
10
down vote

favorite
1












I have implemented a custom iterator to iterate over an array that is held by a pointer. The iterator accepts a unit which specifies by which amount the underlying pointer is increased (or decreased) at each increment (or decrement).



I.e. when



auto data = new int[100];
ArrayIterator<int, false> it(data, 2); // 2nd argument is unit


it + 1 refers to the same element as data + 2.



PointerIterator.hpp



#pragma once

#include <iterator>


/**
* brief A class for iterating over pointers. This class allows to specify a unit, by which the underlying pointers are
* incremented (or decremented) when increasing (or decreasing) it.
* I.e. this allows to easily iterate over columns of a matrix that is represented by a single pointer in row major order.
* tparam DataT The data type the pointer is pointing to.
* tparam Reverse True, if the iterator is used as a reverse iterator (incrementing the iterator is decrementing the pointer, and vice versa).
*/
template <typename DataT, bool Reverse = false>
class ArrayIterator

public:
using difference_type = long long;
using value_type = DataT;
using pointer = DataT*;
using const_pointer = const DataT*;
using reference = DataT&;
using const_reference = const DataT&;
using iterator_category = std::random_access_iterator_tag;

using const_iterator = ArrayIterator<const std::remove_const_t<DataT>, Reverse>;


/**
* brief Creates an invalid iterator.
*/
ArrayIterator() = default;

/**
* brief Creates a pointer iterator for the specified pointer.
* tparam DataT The underlying data type of the pointer.
* param data The pointer.
* param unit The amount by which the pointer is increased (or decreased) at each increment (or decrement).
*/
explicit ArrayIterator(DataT* data, size_t unit = 1);

operator const_iterator() const; // allow implicit conversion from iterator to const iterator

ArrayIterator& operator++();
ArrayIterator operator++(int);

ArrayIterator& operator--();
ArrayIterator operator--(int);

ArrayIterator& operator+=(int n);
ArrayIterator& operator-=(int n);

ArrayIterator operator+(int n) const;

ArrayIterator operator-(int n) const;
difference_type operator-(ArrayIterator other) const;

reference operator(int n);
const_reference operator(int n) const;

bool operator<(ArrayIterator other) const;
bool operator<=(ArrayIterator other) const;
bool operator>(ArrayIterator other) const;
bool operator>=(ArrayIterator other) const;

bool operator==(ArrayIterator other) const;
bool operator!=(ArrayIterator other) const;

pointer operator->();
const_pointer operator->() const;
reference operator*();
const_reference operator*() const;

void swap(ArrayIterator& other) noexcept;
size_t getUnit() const;

private:
pointer _ptr = nullptr;
size_t _unit = 0;
;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>::ArrayIterator(DataT* data, size_t unit)
: _ptr(data), _unit(unit)




template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>::operator const_iterator() const

using type = const std::remove_const_t<DataT>;
return ArrayIterator<type, Reverse>(reinterpret_cast<type*>(_ptr), _unit);


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>& ArrayIterator<DataT, Reverse>::operator++()

if constexpr (Reverse)
_ptr -= _unit;
else
_ptr += _unit;

return *this;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse> ArrayIterator<DataT, Reverse>::operator++(int)

auto it = *this;
++*this;
return it;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>& ArrayIterator<DataT, Reverse>::operator--()

if constexpr (Reverse)
_ptr += _unit;
else
_ptr -= _unit;

return *this;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse> ArrayIterator<DataT, Reverse>::operator--(int)

auto it = *this;
--*this;
return it;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>& ArrayIterator<DataT, Reverse>::operator+=(int n)

if constexpr (Reverse)
_ptr -= n * _unit;
else
_ptr += n * _unit;

return *this;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>& ArrayIterator<DataT, Reverse>::operator-=(int n)

if constexpr (Reverse)
_ptr += n * _unit;
else
_ptr -= n * _unit;

return *this;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse> ArrayIterator<DataT, Reverse>::operator+(int n) const

auto it = *this;
it += n;
return it;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse> ArrayIterator<DataT, Reverse>::operator-(int n) const

auto it = *this;
it -= n;
return it;


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::difference_type ArrayIterator<DataT, Reverse>::operator-(ArrayIterator other) const

if (_unit != other._unit)
throw std::logic_error("Cannot calculate differnce for iterators with different unit.");

if constexpr (Reverse)
return (_ptr - other._ptr) / _unit;
else
return (other._ptr - _ptr) / _unit;


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::reference ArrayIterator<DataT, Reverse>::operator(int n)

return *(*this + n);


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::const_reference ArrayIterator<DataT, Reverse>::operator(int n) const

return *(*this + n);


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator<(ArrayIterator other) const

if constexpr (Reverse)
return other._ptr < _ptr;
else
return _ptr < other._ptr;


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator<=(ArrayIterator other) const

if constexpr (Reverse)
return other._ptr <= _ptr;
else
return _ptr <= other._ptr;


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator>(ArrayIterator other) const

if constexpr (Reverse)
return other._ptr > _ptr;
else
return _ptr > other._ptr;


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator>=(ArrayIterator other) const

if constexpr (Reverse)
return other._ptr >= _ptr;
else
return _ptr >= other._ptr;


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator==(ArrayIterator other) const

return _ptr == other._ptr;


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator!=(ArrayIterator other) const

return !(*this == other);


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::pointer ArrayIterator<DataT, Reverse>::operator->()

return _ptr;


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::const_pointer ArrayIterator<DataT, Reverse>::operator->() const

return _ptr;


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::reference ArrayIterator<DataT, Reverse>::operator*()

return *_ptr;


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::const_reference ArrayIterator<DataT, Reverse>::operator*() const

return *_ptr;


template <typename DataT, bool Reverse>
void ArrayIterator<DataT, Reverse>::swap(ArrayIterator& other) noexcept

auto mPtr = _ptr;
auto mUnit = _unit;
_ptr = other._ptr;
_unit = other._unit;
other._ptr = mPtr;
other._unit = mUnit;


template <typename DataT, bool Reverse>
size_t ArrayIterator<DataT, Reverse>::getUnit() const

return _unit;



/**
* brief Creates an iterator for the specified pointer.
* tparam DataT The underlying data type of the pointer.
* param ptr The pointer.
* param unit The amount by which the pointer is increased (or decreased) at each increment (or decrement).
*/
template <typename DataT>
ArrayIterator<DataT, false> makeArrayIterator(DataT* ptr, size_t unit = 1)

return ArrayIterator<DataT, false>(ptr, unit);



/**
* brief Creates a reversed iterator for the specified pointer. That is, if the iterator is increased by
* an arbitrary amount x, the underlying pointer is decreased by x (and vice versa).
* tparam DataT The underlying data type of the pointer.
* param ptr The pointer.
* param unit The amount by which the pointer is increased (or decreased) at each increment (or decrement).
*/
template <typename DataT>
ArrayIterator<DataT, true> makeArrayIterator(DataT* ptr, size_t unit = 1)

return ArrayIterator<DataT, true>(ptr, unit);


/**
* brief Creates a const iterator for the specified pointer.
* tparam DataT The underlying data type of the pointer.
* param ptr The pointer.
* param unit The amount by which the pointer is increased (or decreased) at each increment (or decrement).
*/
template <typename DataT>
ArrayIterator<DataT, false> makeConstArrayIterator(DataT* ptr, size_t unit = 1)

return ArrayIterator<const std::remove_const_t<DataT>, false>(ptr, unit);



/**
* brief Creates a const reversed iterator for the specified pointer. That is, if the iterator is increased by
* an arbitrary amount x, the underlying pointer is decreased by x (and vice versa).
* tparam DataT The underlying data type of the pointer.
* param ptr The pointer.
* param unit The amount by which the pointer is increased (or decreased) at each increment (or decrement).
*/
template <typename DataT>
ArrayIterator<const DataT, true> makeConstReverseArrayIterator(DataT* ptr, size_t unit = 1)

return ArrayIterator<const std::remove_const_t<DataT>, true>(ptr, unit);



Are there any mistakes or improvements?







share|improve this question

















  • 1




    makeConstArrayIterator with direction forward is missing a const on the 1st template argument for the return type.
    – Darhuuk
    Apr 16 at 13:31











  • @Darhuuk Thanks, fixed it.
    – Timo
    Apr 16 at 13:37






  • 1




    This seems an oddly specific template. A pointer-into-array is a RandomAccessIterator. I would generalise this to adapt any RandomAccessIterator
    – Caleth
    Apr 16 at 17:44










  • @Caleth That is indeed a good point.
    – Timo
    Apr 16 at 18:56










  • Why is units a constructor parameter and not a template parameter. You mention that pointers with different units are not comparable. So why not make them distinct types by putting the size into the template. It also reduces the size of your iterator object.
    – Martin York
    Apr 16 at 22:09

















up vote
10
down vote

favorite
1












I have implemented a custom iterator to iterate over an array that is held by a pointer. The iterator accepts a unit which specifies by which amount the underlying pointer is increased (or decreased) at each increment (or decrement).



I.e. when



auto data = new int[100];
ArrayIterator<int, false> it(data, 2); // 2nd argument is unit


it + 1 refers to the same element as data + 2.



PointerIterator.hpp



#pragma once

#include <iterator>


/**
* brief A class for iterating over pointers. This class allows to specify a unit, by which the underlying pointers are
* incremented (or decremented) when increasing (or decreasing) it.
* I.e. this allows to easily iterate over columns of a matrix that is represented by a single pointer in row major order.
* tparam DataT The data type the pointer is pointing to.
* tparam Reverse True, if the iterator is used as a reverse iterator (incrementing the iterator is decrementing the pointer, and vice versa).
*/
template <typename DataT, bool Reverse = false>
class ArrayIterator

public:
using difference_type = long long;
using value_type = DataT;
using pointer = DataT*;
using const_pointer = const DataT*;
using reference = DataT&;
using const_reference = const DataT&;
using iterator_category = std::random_access_iterator_tag;

using const_iterator = ArrayIterator<const std::remove_const_t<DataT>, Reverse>;


/**
* brief Creates an invalid iterator.
*/
ArrayIterator() = default;

/**
* brief Creates a pointer iterator for the specified pointer.
* tparam DataT The underlying data type of the pointer.
* param data The pointer.
* param unit The amount by which the pointer is increased (or decreased) at each increment (or decrement).
*/
explicit ArrayIterator(DataT* data, size_t unit = 1);

operator const_iterator() const; // allow implicit conversion from iterator to const iterator

ArrayIterator& operator++();
ArrayIterator operator++(int);

ArrayIterator& operator--();
ArrayIterator operator--(int);

ArrayIterator& operator+=(int n);
ArrayIterator& operator-=(int n);

ArrayIterator operator+(int n) const;

ArrayIterator operator-(int n) const;
difference_type operator-(ArrayIterator other) const;

reference operator(int n);
const_reference operator(int n) const;

bool operator<(ArrayIterator other) const;
bool operator<=(ArrayIterator other) const;
bool operator>(ArrayIterator other) const;
bool operator>=(ArrayIterator other) const;

bool operator==(ArrayIterator other) const;
bool operator!=(ArrayIterator other) const;

pointer operator->();
const_pointer operator->() const;
reference operator*();
const_reference operator*() const;

void swap(ArrayIterator& other) noexcept;
size_t getUnit() const;

private:
pointer _ptr = nullptr;
size_t _unit = 0;
;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>::ArrayIterator(DataT* data, size_t unit)
: _ptr(data), _unit(unit)




template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>::operator const_iterator() const

using type = const std::remove_const_t<DataT>;
return ArrayIterator<type, Reverse>(reinterpret_cast<type*>(_ptr), _unit);


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>& ArrayIterator<DataT, Reverse>::operator++()

if constexpr (Reverse)
_ptr -= _unit;
else
_ptr += _unit;

return *this;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse> ArrayIterator<DataT, Reverse>::operator++(int)

auto it = *this;
++*this;
return it;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>& ArrayIterator<DataT, Reverse>::operator--()

if constexpr (Reverse)
_ptr += _unit;
else
_ptr -= _unit;

return *this;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse> ArrayIterator<DataT, Reverse>::operator--(int)

auto it = *this;
--*this;
return it;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>& ArrayIterator<DataT, Reverse>::operator+=(int n)

if constexpr (Reverse)
_ptr -= n * _unit;
else
_ptr += n * _unit;

return *this;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>& ArrayIterator<DataT, Reverse>::operator-=(int n)

if constexpr (Reverse)
_ptr += n * _unit;
else
_ptr -= n * _unit;

return *this;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse> ArrayIterator<DataT, Reverse>::operator+(int n) const

auto it = *this;
it += n;
return it;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse> ArrayIterator<DataT, Reverse>::operator-(int n) const

auto it = *this;
it -= n;
return it;


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::difference_type ArrayIterator<DataT, Reverse>::operator-(ArrayIterator other) const

if (_unit != other._unit)
throw std::logic_error("Cannot calculate differnce for iterators with different unit.");

if constexpr (Reverse)
return (_ptr - other._ptr) / _unit;
else
return (other._ptr - _ptr) / _unit;


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::reference ArrayIterator<DataT, Reverse>::operator(int n)

return *(*this + n);


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::const_reference ArrayIterator<DataT, Reverse>::operator(int n) const

return *(*this + n);


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator<(ArrayIterator other) const

if constexpr (Reverse)
return other._ptr < _ptr;
else
return _ptr < other._ptr;


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator<=(ArrayIterator other) const

if constexpr (Reverse)
return other._ptr <= _ptr;
else
return _ptr <= other._ptr;


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator>(ArrayIterator other) const

if constexpr (Reverse)
return other._ptr > _ptr;
else
return _ptr > other._ptr;


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator>=(ArrayIterator other) const

if constexpr (Reverse)
return other._ptr >= _ptr;
else
return _ptr >= other._ptr;


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator==(ArrayIterator other) const

return _ptr == other._ptr;


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator!=(ArrayIterator other) const

return !(*this == other);


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::pointer ArrayIterator<DataT, Reverse>::operator->()

return _ptr;


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::const_pointer ArrayIterator<DataT, Reverse>::operator->() const

return _ptr;


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::reference ArrayIterator<DataT, Reverse>::operator*()

return *_ptr;


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::const_reference ArrayIterator<DataT, Reverse>::operator*() const

return *_ptr;


template <typename DataT, bool Reverse>
void ArrayIterator<DataT, Reverse>::swap(ArrayIterator& other) noexcept

auto mPtr = _ptr;
auto mUnit = _unit;
_ptr = other._ptr;
_unit = other._unit;
other._ptr = mPtr;
other._unit = mUnit;


template <typename DataT, bool Reverse>
size_t ArrayIterator<DataT, Reverse>::getUnit() const

return _unit;



/**
* brief Creates an iterator for the specified pointer.
* tparam DataT The underlying data type of the pointer.
* param ptr The pointer.
* param unit The amount by which the pointer is increased (or decreased) at each increment (or decrement).
*/
template <typename DataT>
ArrayIterator<DataT, false> makeArrayIterator(DataT* ptr, size_t unit = 1)

return ArrayIterator<DataT, false>(ptr, unit);



/**
* brief Creates a reversed iterator for the specified pointer. That is, if the iterator is increased by
* an arbitrary amount x, the underlying pointer is decreased by x (and vice versa).
* tparam DataT The underlying data type of the pointer.
* param ptr The pointer.
* param unit The amount by which the pointer is increased (or decreased) at each increment (or decrement).
*/
template <typename DataT>
ArrayIterator<DataT, true> makeArrayIterator(DataT* ptr, size_t unit = 1)

return ArrayIterator<DataT, true>(ptr, unit);


/**
* brief Creates a const iterator for the specified pointer.
* tparam DataT The underlying data type of the pointer.
* param ptr The pointer.
* param unit The amount by which the pointer is increased (or decreased) at each increment (or decrement).
*/
template <typename DataT>
ArrayIterator<DataT, false> makeConstArrayIterator(DataT* ptr, size_t unit = 1)

return ArrayIterator<const std::remove_const_t<DataT>, false>(ptr, unit);



/**
* brief Creates a const reversed iterator for the specified pointer. That is, if the iterator is increased by
* an arbitrary amount x, the underlying pointer is decreased by x (and vice versa).
* tparam DataT The underlying data type of the pointer.
* param ptr The pointer.
* param unit The amount by which the pointer is increased (or decreased) at each increment (or decrement).
*/
template <typename DataT>
ArrayIterator<const DataT, true> makeConstReverseArrayIterator(DataT* ptr, size_t unit = 1)

return ArrayIterator<const std::remove_const_t<DataT>, true>(ptr, unit);



Are there any mistakes or improvements?







share|improve this question

















  • 1




    makeConstArrayIterator with direction forward is missing a const on the 1st template argument for the return type.
    – Darhuuk
    Apr 16 at 13:31











  • @Darhuuk Thanks, fixed it.
    – Timo
    Apr 16 at 13:37






  • 1




    This seems an oddly specific template. A pointer-into-array is a RandomAccessIterator. I would generalise this to adapt any RandomAccessIterator
    – Caleth
    Apr 16 at 17:44










  • @Caleth That is indeed a good point.
    – Timo
    Apr 16 at 18:56










  • Why is units a constructor parameter and not a template parameter. You mention that pointers with different units are not comparable. So why not make them distinct types by putting the size into the template. It also reduces the size of your iterator object.
    – Martin York
    Apr 16 at 22:09













up vote
10
down vote

favorite
1









up vote
10
down vote

favorite
1






1





I have implemented a custom iterator to iterate over an array that is held by a pointer. The iterator accepts a unit which specifies by which amount the underlying pointer is increased (or decreased) at each increment (or decrement).



I.e. when



auto data = new int[100];
ArrayIterator<int, false> it(data, 2); // 2nd argument is unit


it + 1 refers to the same element as data + 2.



PointerIterator.hpp



#pragma once

#include <iterator>


/**
* brief A class for iterating over pointers. This class allows to specify a unit, by which the underlying pointers are
* incremented (or decremented) when increasing (or decreasing) it.
* I.e. this allows to easily iterate over columns of a matrix that is represented by a single pointer in row major order.
* tparam DataT The data type the pointer is pointing to.
* tparam Reverse True, if the iterator is used as a reverse iterator (incrementing the iterator is decrementing the pointer, and vice versa).
*/
template <typename DataT, bool Reverse = false>
class ArrayIterator

public:
using difference_type = long long;
using value_type = DataT;
using pointer = DataT*;
using const_pointer = const DataT*;
using reference = DataT&;
using const_reference = const DataT&;
using iterator_category = std::random_access_iterator_tag;

using const_iterator = ArrayIterator<const std::remove_const_t<DataT>, Reverse>;


/**
* brief Creates an invalid iterator.
*/
ArrayIterator() = default;

/**
* brief Creates a pointer iterator for the specified pointer.
* tparam DataT The underlying data type of the pointer.
* param data The pointer.
* param unit The amount by which the pointer is increased (or decreased) at each increment (or decrement).
*/
explicit ArrayIterator(DataT* data, size_t unit = 1);

operator const_iterator() const; // allow implicit conversion from iterator to const iterator

ArrayIterator& operator++();
ArrayIterator operator++(int);

ArrayIterator& operator--();
ArrayIterator operator--(int);

ArrayIterator& operator+=(int n);
ArrayIterator& operator-=(int n);

ArrayIterator operator+(int n) const;

ArrayIterator operator-(int n) const;
difference_type operator-(ArrayIterator other) const;

reference operator(int n);
const_reference operator(int n) const;

bool operator<(ArrayIterator other) const;
bool operator<=(ArrayIterator other) const;
bool operator>(ArrayIterator other) const;
bool operator>=(ArrayIterator other) const;

bool operator==(ArrayIterator other) const;
bool operator!=(ArrayIterator other) const;

pointer operator->();
const_pointer operator->() const;
reference operator*();
const_reference operator*() const;

void swap(ArrayIterator& other) noexcept;
size_t getUnit() const;

private:
pointer _ptr = nullptr;
size_t _unit = 0;
;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>::ArrayIterator(DataT* data, size_t unit)
: _ptr(data), _unit(unit)




template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>::operator const_iterator() const

using type = const std::remove_const_t<DataT>;
return ArrayIterator<type, Reverse>(reinterpret_cast<type*>(_ptr), _unit);


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>& ArrayIterator<DataT, Reverse>::operator++()

if constexpr (Reverse)
_ptr -= _unit;
else
_ptr += _unit;

return *this;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse> ArrayIterator<DataT, Reverse>::operator++(int)

auto it = *this;
++*this;
return it;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>& ArrayIterator<DataT, Reverse>::operator--()

if constexpr (Reverse)
_ptr += _unit;
else
_ptr -= _unit;

return *this;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse> ArrayIterator<DataT, Reverse>::operator--(int)

auto it = *this;
--*this;
return it;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>& ArrayIterator<DataT, Reverse>::operator+=(int n)

if constexpr (Reverse)
_ptr -= n * _unit;
else
_ptr += n * _unit;

return *this;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>& ArrayIterator<DataT, Reverse>::operator-=(int n)

if constexpr (Reverse)
_ptr += n * _unit;
else
_ptr -= n * _unit;

return *this;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse> ArrayIterator<DataT, Reverse>::operator+(int n) const

auto it = *this;
it += n;
return it;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse> ArrayIterator<DataT, Reverse>::operator-(int n) const

auto it = *this;
it -= n;
return it;


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::difference_type ArrayIterator<DataT, Reverse>::operator-(ArrayIterator other) const

if (_unit != other._unit)
throw std::logic_error("Cannot calculate differnce for iterators with different unit.");

if constexpr (Reverse)
return (_ptr - other._ptr) / _unit;
else
return (other._ptr - _ptr) / _unit;


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::reference ArrayIterator<DataT, Reverse>::operator(int n)

return *(*this + n);


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::const_reference ArrayIterator<DataT, Reverse>::operator(int n) const

return *(*this + n);


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator<(ArrayIterator other) const

if constexpr (Reverse)
return other._ptr < _ptr;
else
return _ptr < other._ptr;


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator<=(ArrayIterator other) const

if constexpr (Reverse)
return other._ptr <= _ptr;
else
return _ptr <= other._ptr;


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator>(ArrayIterator other) const

if constexpr (Reverse)
return other._ptr > _ptr;
else
return _ptr > other._ptr;


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator>=(ArrayIterator other) const

if constexpr (Reverse)
return other._ptr >= _ptr;
else
return _ptr >= other._ptr;


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator==(ArrayIterator other) const

return _ptr == other._ptr;


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator!=(ArrayIterator other) const

return !(*this == other);


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::pointer ArrayIterator<DataT, Reverse>::operator->()

return _ptr;


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::const_pointer ArrayIterator<DataT, Reverse>::operator->() const

return _ptr;


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::reference ArrayIterator<DataT, Reverse>::operator*()

return *_ptr;


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::const_reference ArrayIterator<DataT, Reverse>::operator*() const

return *_ptr;


template <typename DataT, bool Reverse>
void ArrayIterator<DataT, Reverse>::swap(ArrayIterator& other) noexcept

auto mPtr = _ptr;
auto mUnit = _unit;
_ptr = other._ptr;
_unit = other._unit;
other._ptr = mPtr;
other._unit = mUnit;


template <typename DataT, bool Reverse>
size_t ArrayIterator<DataT, Reverse>::getUnit() const

return _unit;



/**
* brief Creates an iterator for the specified pointer.
* tparam DataT The underlying data type of the pointer.
* param ptr The pointer.
* param unit The amount by which the pointer is increased (or decreased) at each increment (or decrement).
*/
template <typename DataT>
ArrayIterator<DataT, false> makeArrayIterator(DataT* ptr, size_t unit = 1)

return ArrayIterator<DataT, false>(ptr, unit);



/**
* brief Creates a reversed iterator for the specified pointer. That is, if the iterator is increased by
* an arbitrary amount x, the underlying pointer is decreased by x (and vice versa).
* tparam DataT The underlying data type of the pointer.
* param ptr The pointer.
* param unit The amount by which the pointer is increased (or decreased) at each increment (or decrement).
*/
template <typename DataT>
ArrayIterator<DataT, true> makeArrayIterator(DataT* ptr, size_t unit = 1)

return ArrayIterator<DataT, true>(ptr, unit);


/**
* brief Creates a const iterator for the specified pointer.
* tparam DataT The underlying data type of the pointer.
* param ptr The pointer.
* param unit The amount by which the pointer is increased (or decreased) at each increment (or decrement).
*/
template <typename DataT>
ArrayIterator<DataT, false> makeConstArrayIterator(DataT* ptr, size_t unit = 1)

return ArrayIterator<const std::remove_const_t<DataT>, false>(ptr, unit);



/**
* brief Creates a const reversed iterator for the specified pointer. That is, if the iterator is increased by
* an arbitrary amount x, the underlying pointer is decreased by x (and vice versa).
* tparam DataT The underlying data type of the pointer.
* param ptr The pointer.
* param unit The amount by which the pointer is increased (or decreased) at each increment (or decrement).
*/
template <typename DataT>
ArrayIterator<const DataT, true> makeConstReverseArrayIterator(DataT* ptr, size_t unit = 1)

return ArrayIterator<const std::remove_const_t<DataT>, true>(ptr, unit);



Are there any mistakes or improvements?







share|improve this question













I have implemented a custom iterator to iterate over an array that is held by a pointer. The iterator accepts a unit which specifies by which amount the underlying pointer is increased (or decreased) at each increment (or decrement).



I.e. when



auto data = new int[100];
ArrayIterator<int, false> it(data, 2); // 2nd argument is unit


it + 1 refers to the same element as data + 2.



PointerIterator.hpp



#pragma once

#include <iterator>


/**
* brief A class for iterating over pointers. This class allows to specify a unit, by which the underlying pointers are
* incremented (or decremented) when increasing (or decreasing) it.
* I.e. this allows to easily iterate over columns of a matrix that is represented by a single pointer in row major order.
* tparam DataT The data type the pointer is pointing to.
* tparam Reverse True, if the iterator is used as a reverse iterator (incrementing the iterator is decrementing the pointer, and vice versa).
*/
template <typename DataT, bool Reverse = false>
class ArrayIterator

public:
using difference_type = long long;
using value_type = DataT;
using pointer = DataT*;
using const_pointer = const DataT*;
using reference = DataT&;
using const_reference = const DataT&;
using iterator_category = std::random_access_iterator_tag;

using const_iterator = ArrayIterator<const std::remove_const_t<DataT>, Reverse>;


/**
* brief Creates an invalid iterator.
*/
ArrayIterator() = default;

/**
* brief Creates a pointer iterator for the specified pointer.
* tparam DataT The underlying data type of the pointer.
* param data The pointer.
* param unit The amount by which the pointer is increased (or decreased) at each increment (or decrement).
*/
explicit ArrayIterator(DataT* data, size_t unit = 1);

operator const_iterator() const; // allow implicit conversion from iterator to const iterator

ArrayIterator& operator++();
ArrayIterator operator++(int);

ArrayIterator& operator--();
ArrayIterator operator--(int);

ArrayIterator& operator+=(int n);
ArrayIterator& operator-=(int n);

ArrayIterator operator+(int n) const;

ArrayIterator operator-(int n) const;
difference_type operator-(ArrayIterator other) const;

reference operator(int n);
const_reference operator(int n) const;

bool operator<(ArrayIterator other) const;
bool operator<=(ArrayIterator other) const;
bool operator>(ArrayIterator other) const;
bool operator>=(ArrayIterator other) const;

bool operator==(ArrayIterator other) const;
bool operator!=(ArrayIterator other) const;

pointer operator->();
const_pointer operator->() const;
reference operator*();
const_reference operator*() const;

void swap(ArrayIterator& other) noexcept;
size_t getUnit() const;

private:
pointer _ptr = nullptr;
size_t _unit = 0;
;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>::ArrayIterator(DataT* data, size_t unit)
: _ptr(data), _unit(unit)




template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>::operator const_iterator() const

using type = const std::remove_const_t<DataT>;
return ArrayIterator<type, Reverse>(reinterpret_cast<type*>(_ptr), _unit);


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>& ArrayIterator<DataT, Reverse>::operator++()

if constexpr (Reverse)
_ptr -= _unit;
else
_ptr += _unit;

return *this;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse> ArrayIterator<DataT, Reverse>::operator++(int)

auto it = *this;
++*this;
return it;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>& ArrayIterator<DataT, Reverse>::operator--()

if constexpr (Reverse)
_ptr += _unit;
else
_ptr -= _unit;

return *this;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse> ArrayIterator<DataT, Reverse>::operator--(int)

auto it = *this;
--*this;
return it;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>& ArrayIterator<DataT, Reverse>::operator+=(int n)

if constexpr (Reverse)
_ptr -= n * _unit;
else
_ptr += n * _unit;

return *this;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse>& ArrayIterator<DataT, Reverse>::operator-=(int n)

if constexpr (Reverse)
_ptr += n * _unit;
else
_ptr -= n * _unit;

return *this;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse> ArrayIterator<DataT, Reverse>::operator+(int n) const

auto it = *this;
it += n;
return it;


template <typename DataT, bool Reverse>
ArrayIterator<DataT, Reverse> ArrayIterator<DataT, Reverse>::operator-(int n) const

auto it = *this;
it -= n;
return it;


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::difference_type ArrayIterator<DataT, Reverse>::operator-(ArrayIterator other) const

if (_unit != other._unit)
throw std::logic_error("Cannot calculate differnce for iterators with different unit.");

if constexpr (Reverse)
return (_ptr - other._ptr) / _unit;
else
return (other._ptr - _ptr) / _unit;


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::reference ArrayIterator<DataT, Reverse>::operator(int n)

return *(*this + n);


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::const_reference ArrayIterator<DataT, Reverse>::operator(int n) const

return *(*this + n);


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator<(ArrayIterator other) const

if constexpr (Reverse)
return other._ptr < _ptr;
else
return _ptr < other._ptr;


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator<=(ArrayIterator other) const

if constexpr (Reverse)
return other._ptr <= _ptr;
else
return _ptr <= other._ptr;


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator>(ArrayIterator other) const

if constexpr (Reverse)
return other._ptr > _ptr;
else
return _ptr > other._ptr;


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator>=(ArrayIterator other) const

if constexpr (Reverse)
return other._ptr >= _ptr;
else
return _ptr >= other._ptr;


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator==(ArrayIterator other) const

return _ptr == other._ptr;


template <typename DataT, bool Reverse>
bool ArrayIterator<DataT, Reverse>::operator!=(ArrayIterator other) const

return !(*this == other);


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::pointer ArrayIterator<DataT, Reverse>::operator->()

return _ptr;


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::const_pointer ArrayIterator<DataT, Reverse>::operator->() const

return _ptr;


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::reference ArrayIterator<DataT, Reverse>::operator*()

return *_ptr;


template <typename DataT, bool Reverse>
typename ArrayIterator<DataT, Reverse>::const_reference ArrayIterator<DataT, Reverse>::operator*() const

return *_ptr;


template <typename DataT, bool Reverse>
void ArrayIterator<DataT, Reverse>::swap(ArrayIterator& other) noexcept

auto mPtr = _ptr;
auto mUnit = _unit;
_ptr = other._ptr;
_unit = other._unit;
other._ptr = mPtr;
other._unit = mUnit;


template <typename DataT, bool Reverse>
size_t ArrayIterator<DataT, Reverse>::getUnit() const

return _unit;



/**
* brief Creates an iterator for the specified pointer.
* tparam DataT The underlying data type of the pointer.
* param ptr The pointer.
* param unit The amount by which the pointer is increased (or decreased) at each increment (or decrement).
*/
template <typename DataT>
ArrayIterator<DataT, false> makeArrayIterator(DataT* ptr, size_t unit = 1)

return ArrayIterator<DataT, false>(ptr, unit);



/**
* brief Creates a reversed iterator for the specified pointer. That is, if the iterator is increased by
* an arbitrary amount x, the underlying pointer is decreased by x (and vice versa).
* tparam DataT The underlying data type of the pointer.
* param ptr The pointer.
* param unit The amount by which the pointer is increased (or decreased) at each increment (or decrement).
*/
template <typename DataT>
ArrayIterator<DataT, true> makeArrayIterator(DataT* ptr, size_t unit = 1)

return ArrayIterator<DataT, true>(ptr, unit);


/**
* brief Creates a const iterator for the specified pointer.
* tparam DataT The underlying data type of the pointer.
* param ptr The pointer.
* param unit The amount by which the pointer is increased (or decreased) at each increment (or decrement).
*/
template <typename DataT>
ArrayIterator<DataT, false> makeConstArrayIterator(DataT* ptr, size_t unit = 1)

return ArrayIterator<const std::remove_const_t<DataT>, false>(ptr, unit);



/**
* brief Creates a const reversed iterator for the specified pointer. That is, if the iterator is increased by
* an arbitrary amount x, the underlying pointer is decreased by x (and vice versa).
* tparam DataT The underlying data type of the pointer.
* param ptr The pointer.
* param unit The amount by which the pointer is increased (or decreased) at each increment (or decrement).
*/
template <typename DataT>
ArrayIterator<const DataT, true> makeConstReverseArrayIterator(DataT* ptr, size_t unit = 1)

return ArrayIterator<const std::remove_const_t<DataT>, true>(ptr, unit);



Are there any mistakes or improvements?









share|improve this question












share|improve this question




share|improve this question








edited Apr 16 at 15:17
























asked Apr 16 at 12:05









Timo

1818




1818







  • 1




    makeConstArrayIterator with direction forward is missing a const on the 1st template argument for the return type.
    – Darhuuk
    Apr 16 at 13:31











  • @Darhuuk Thanks, fixed it.
    – Timo
    Apr 16 at 13:37






  • 1




    This seems an oddly specific template. A pointer-into-array is a RandomAccessIterator. I would generalise this to adapt any RandomAccessIterator
    – Caleth
    Apr 16 at 17:44










  • @Caleth That is indeed a good point.
    – Timo
    Apr 16 at 18:56










  • Why is units a constructor parameter and not a template parameter. You mention that pointers with different units are not comparable. So why not make them distinct types by putting the size into the template. It also reduces the size of your iterator object.
    – Martin York
    Apr 16 at 22:09













  • 1




    makeConstArrayIterator with direction forward is missing a const on the 1st template argument for the return type.
    – Darhuuk
    Apr 16 at 13:31











  • @Darhuuk Thanks, fixed it.
    – Timo
    Apr 16 at 13:37






  • 1




    This seems an oddly specific template. A pointer-into-array is a RandomAccessIterator. I would generalise this to adapt any RandomAccessIterator
    – Caleth
    Apr 16 at 17:44










  • @Caleth That is indeed a good point.
    – Timo
    Apr 16 at 18:56










  • Why is units a constructor parameter and not a template parameter. You mention that pointers with different units are not comparable. So why not make them distinct types by putting the size into the template. It also reduces the size of your iterator object.
    – Martin York
    Apr 16 at 22:09








1




1




makeConstArrayIterator with direction forward is missing a const on the 1st template argument for the return type.
– Darhuuk
Apr 16 at 13:31





makeConstArrayIterator with direction forward is missing a const on the 1st template argument for the return type.
– Darhuuk
Apr 16 at 13:31













@Darhuuk Thanks, fixed it.
– Timo
Apr 16 at 13:37




@Darhuuk Thanks, fixed it.
– Timo
Apr 16 at 13:37




1




1




This seems an oddly specific template. A pointer-into-array is a RandomAccessIterator. I would generalise this to adapt any RandomAccessIterator
– Caleth
Apr 16 at 17:44




This seems an oddly specific template. A pointer-into-array is a RandomAccessIterator. I would generalise this to adapt any RandomAccessIterator
– Caleth
Apr 16 at 17:44












@Caleth That is indeed a good point.
– Timo
Apr 16 at 18:56




@Caleth That is indeed a good point.
– Timo
Apr 16 at 18:56












Why is units a constructor parameter and not a template parameter. You mention that pointers with different units are not comparable. So why not make them distinct types by putting the size into the template. It also reduces the size of your iterator object.
– Martin York
Apr 16 at 22:09





Why is units a constructor parameter and not a template parameter. You mention that pointers with different units are not comparable. So why not make them distinct types by putting the size into the template. It also reduces the size of your iterator object.
– Martin York
Apr 16 at 22:09











2 Answers
2






active

oldest

votes

















up vote
8
down vote



accepted










Design Issues



If there is one thing that bothers me about your code, it's that Reverse template parameter. First off all, why would you code that into your class if you could just use std::reverse_iterator and be done?



Secondly, I would argue against adding additional functionality to iterators. After all, the reason for having iterators in the first place is to have a uniform, pointer-like access to the contents of a collection. Adding additional functionality into this interface violates the principle of minimalism that the design is based on.



Other Things



  1. Fix your includes. You're missing #include <type_traits> (for std::remove_const_t), #include <stdexcept> (for std::logic_error) and #include <cstddef> or similar (for std::size_t).

  2. Don't rely on C legacy types being defined in the global namespace. ::size_t may not exist; use std::size_t instead.


  3. using difference_type = long long; should be using difference_type = std::ptrdiff_t. The latter is arguably more correct here and expresses your intents better.

  4. Why do you take an int as argument for operator? This limits your dereference to the size of int, whereas the underlying container can have a size of std::size_t.

  5. It appears you have typo'ed "difference" in throw std::logic_error("Cannot calculate differnce for iterators with different unit.");.





share|improve this answer





















  • I completely forgot about std::reverse_iterator. Thanks for pointing that out. About your second design issue: what additional functionality are you referring to? The only overhead I can see is getUnit(), which I thought would be helpful if it's accessible from outside. Other Things: 3: I thought I read on cppreference that std::ptrdiff_t is marked as deprecated in C++17, but looks like I messed that up. I will fix the other points asap.
    – Timo
    Apr 16 at 14:51










  • @Timo I was referring to the reverse_iterator issue with my second design point as well, so I guess you don't need to consider that anymore.
    – Ben Steffan
    Apr 16 at 15:01










  • Alright. I think I fixed everything. Can you confirm, please?
    – Timo
    Apr 16 at 15:06










  • @Timo I'm sorry, but you will have to roll back your edit. The validity and usefulness of answers on Code Review depends on code in questions not being edited after there is at least one review. If you still have doubts about your code, please post a follow up question instead.
    – Ben Steffan
    Apr 16 at 15:13






  • 1




    @yuri, for standard library it is not hard to remember (may be if you're working on a project with bizillion includes). There are off course edge cases, which for me is multithreading stuff.
    – Incomputable
    Apr 16 at 16:25

















up vote
0
down vote













Take a look at Boost.Iterator.



You can write what you want by a simple use of Boost’s iterator_adaptor, changing only what you need to change (e.g. advance member). Seriously, it does all that code you wrote, already in the can and well reviewed for edge cases. You only have to write what’s different from the underlying iterator, and everything else comes through.



Or, if you’re living on the bleeding edge, just use a view::stride in the Range v3 library.




Given a source range and an integral stride value, return a range consisting of every Nth element, starting with the first.







share|improve this answer





















    Your Answer




    StackExchange.ifUsing("editor", function ()
    return StackExchange.using("mathjaxEditing", function ()
    StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
    StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
    );
    );
    , "mathjax-editing");

    StackExchange.ifUsing("editor", function ()
    StackExchange.using("externalEditor", function ()
    StackExchange.using("snippets", function ()
    StackExchange.snippets.init();
    );
    );
    , "code-snippets");

    StackExchange.ready(function()
    var channelOptions =
    tags: "".split(" "),
    id: "196"
    ;
    initTagRenderer("".split(" "), "".split(" "), channelOptions);

    StackExchange.using("externalEditor", function()
    // Have to fire editor after snippets, if snippets enabled
    if (StackExchange.settings.snippets.snippetsEnabled)
    StackExchange.using("snippets", function()
    createEditor();
    );

    else
    createEditor();

    );

    function createEditor()
    StackExchange.prepareEditor(
    heartbeatType: 'answer',
    convertImagesToLinks: false,
    noModals: false,
    showLowRepImageUploadWarning: true,
    reputationToPostImages: null,
    bindNavPrevention: true,
    postfix: "",
    onDemand: true,
    discardSelector: ".discard-answer"
    ,immediatelyShowMarkdownHelp:true
    );



    );








     

    draft saved


    draft discarded


















    StackExchange.ready(
    function ()
    StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f192195%2fcustom-c-random-access-iterator-for-pointers-to-arrays%23new-answer', 'question_page');

    );

    Post as a guest






























    2 Answers
    2






    active

    oldest

    votes








    2 Answers
    2






    active

    oldest

    votes









    active

    oldest

    votes






    active

    oldest

    votes








    up vote
    8
    down vote



    accepted










    Design Issues



    If there is one thing that bothers me about your code, it's that Reverse template parameter. First off all, why would you code that into your class if you could just use std::reverse_iterator and be done?



    Secondly, I would argue against adding additional functionality to iterators. After all, the reason for having iterators in the first place is to have a uniform, pointer-like access to the contents of a collection. Adding additional functionality into this interface violates the principle of minimalism that the design is based on.



    Other Things



    1. Fix your includes. You're missing #include <type_traits> (for std::remove_const_t), #include <stdexcept> (for std::logic_error) and #include <cstddef> or similar (for std::size_t).

    2. Don't rely on C legacy types being defined in the global namespace. ::size_t may not exist; use std::size_t instead.


    3. using difference_type = long long; should be using difference_type = std::ptrdiff_t. The latter is arguably more correct here and expresses your intents better.

    4. Why do you take an int as argument for operator? This limits your dereference to the size of int, whereas the underlying container can have a size of std::size_t.

    5. It appears you have typo'ed "difference" in throw std::logic_error("Cannot calculate differnce for iterators with different unit.");.





    share|improve this answer





















    • I completely forgot about std::reverse_iterator. Thanks for pointing that out. About your second design issue: what additional functionality are you referring to? The only overhead I can see is getUnit(), which I thought would be helpful if it's accessible from outside. Other Things: 3: I thought I read on cppreference that std::ptrdiff_t is marked as deprecated in C++17, but looks like I messed that up. I will fix the other points asap.
      – Timo
      Apr 16 at 14:51










    • @Timo I was referring to the reverse_iterator issue with my second design point as well, so I guess you don't need to consider that anymore.
      – Ben Steffan
      Apr 16 at 15:01










    • Alright. I think I fixed everything. Can you confirm, please?
      – Timo
      Apr 16 at 15:06










    • @Timo I'm sorry, but you will have to roll back your edit. The validity and usefulness of answers on Code Review depends on code in questions not being edited after there is at least one review. If you still have doubts about your code, please post a follow up question instead.
      – Ben Steffan
      Apr 16 at 15:13






    • 1




      @yuri, for standard library it is not hard to remember (may be if you're working on a project with bizillion includes). There are off course edge cases, which for me is multithreading stuff.
      – Incomputable
      Apr 16 at 16:25














    up vote
    8
    down vote



    accepted










    Design Issues



    If there is one thing that bothers me about your code, it's that Reverse template parameter. First off all, why would you code that into your class if you could just use std::reverse_iterator and be done?



    Secondly, I would argue against adding additional functionality to iterators. After all, the reason for having iterators in the first place is to have a uniform, pointer-like access to the contents of a collection. Adding additional functionality into this interface violates the principle of minimalism that the design is based on.



    Other Things



    1. Fix your includes. You're missing #include <type_traits> (for std::remove_const_t), #include <stdexcept> (for std::logic_error) and #include <cstddef> or similar (for std::size_t).

    2. Don't rely on C legacy types being defined in the global namespace. ::size_t may not exist; use std::size_t instead.


    3. using difference_type = long long; should be using difference_type = std::ptrdiff_t. The latter is arguably more correct here and expresses your intents better.

    4. Why do you take an int as argument for operator? This limits your dereference to the size of int, whereas the underlying container can have a size of std::size_t.

    5. It appears you have typo'ed "difference" in throw std::logic_error("Cannot calculate differnce for iterators with different unit.");.





    share|improve this answer





















    • I completely forgot about std::reverse_iterator. Thanks for pointing that out. About your second design issue: what additional functionality are you referring to? The only overhead I can see is getUnit(), which I thought would be helpful if it's accessible from outside. Other Things: 3: I thought I read on cppreference that std::ptrdiff_t is marked as deprecated in C++17, but looks like I messed that up. I will fix the other points asap.
      – Timo
      Apr 16 at 14:51










    • @Timo I was referring to the reverse_iterator issue with my second design point as well, so I guess you don't need to consider that anymore.
      – Ben Steffan
      Apr 16 at 15:01










    • Alright. I think I fixed everything. Can you confirm, please?
      – Timo
      Apr 16 at 15:06










    • @Timo I'm sorry, but you will have to roll back your edit. The validity and usefulness of answers on Code Review depends on code in questions not being edited after there is at least one review. If you still have doubts about your code, please post a follow up question instead.
      – Ben Steffan
      Apr 16 at 15:13






    • 1




      @yuri, for standard library it is not hard to remember (may be if you're working on a project with bizillion includes). There are off course edge cases, which for me is multithreading stuff.
      – Incomputable
      Apr 16 at 16:25












    up vote
    8
    down vote



    accepted







    up vote
    8
    down vote



    accepted






    Design Issues



    If there is one thing that bothers me about your code, it's that Reverse template parameter. First off all, why would you code that into your class if you could just use std::reverse_iterator and be done?



    Secondly, I would argue against adding additional functionality to iterators. After all, the reason for having iterators in the first place is to have a uniform, pointer-like access to the contents of a collection. Adding additional functionality into this interface violates the principle of minimalism that the design is based on.



    Other Things



    1. Fix your includes. You're missing #include <type_traits> (for std::remove_const_t), #include <stdexcept> (for std::logic_error) and #include <cstddef> or similar (for std::size_t).

    2. Don't rely on C legacy types being defined in the global namespace. ::size_t may not exist; use std::size_t instead.


    3. using difference_type = long long; should be using difference_type = std::ptrdiff_t. The latter is arguably more correct here and expresses your intents better.

    4. Why do you take an int as argument for operator? This limits your dereference to the size of int, whereas the underlying container can have a size of std::size_t.

    5. It appears you have typo'ed "difference" in throw std::logic_error("Cannot calculate differnce for iterators with different unit.");.





    share|improve this answer













    Design Issues



    If there is one thing that bothers me about your code, it's that Reverse template parameter. First off all, why would you code that into your class if you could just use std::reverse_iterator and be done?



    Secondly, I would argue against adding additional functionality to iterators. After all, the reason for having iterators in the first place is to have a uniform, pointer-like access to the contents of a collection. Adding additional functionality into this interface violates the principle of minimalism that the design is based on.



    Other Things



    1. Fix your includes. You're missing #include <type_traits> (for std::remove_const_t), #include <stdexcept> (for std::logic_error) and #include <cstddef> or similar (for std::size_t).

    2. Don't rely on C legacy types being defined in the global namespace. ::size_t may not exist; use std::size_t instead.


    3. using difference_type = long long; should be using difference_type = std::ptrdiff_t. The latter is arguably more correct here and expresses your intents better.

    4. Why do you take an int as argument for operator? This limits your dereference to the size of int, whereas the underlying container can have a size of std::size_t.

    5. It appears you have typo'ed "difference" in throw std::logic_error("Cannot calculate differnce for iterators with different unit.");.






    share|improve this answer













    share|improve this answer



    share|improve this answer











    answered Apr 16 at 14:35









    Ben Steffan

    4,85011234




    4,85011234











    • I completely forgot about std::reverse_iterator. Thanks for pointing that out. About your second design issue: what additional functionality are you referring to? The only overhead I can see is getUnit(), which I thought would be helpful if it's accessible from outside. Other Things: 3: I thought I read on cppreference that std::ptrdiff_t is marked as deprecated in C++17, but looks like I messed that up. I will fix the other points asap.
      – Timo
      Apr 16 at 14:51










    • @Timo I was referring to the reverse_iterator issue with my second design point as well, so I guess you don't need to consider that anymore.
      – Ben Steffan
      Apr 16 at 15:01










    • Alright. I think I fixed everything. Can you confirm, please?
      – Timo
      Apr 16 at 15:06










    • @Timo I'm sorry, but you will have to roll back your edit. The validity and usefulness of answers on Code Review depends on code in questions not being edited after there is at least one review. If you still have doubts about your code, please post a follow up question instead.
      – Ben Steffan
      Apr 16 at 15:13






    • 1




      @yuri, for standard library it is not hard to remember (may be if you're working on a project with bizillion includes). There are off course edge cases, which for me is multithreading stuff.
      – Incomputable
      Apr 16 at 16:25
















    • I completely forgot about std::reverse_iterator. Thanks for pointing that out. About your second design issue: what additional functionality are you referring to? The only overhead I can see is getUnit(), which I thought would be helpful if it's accessible from outside. Other Things: 3: I thought I read on cppreference that std::ptrdiff_t is marked as deprecated in C++17, but looks like I messed that up. I will fix the other points asap.
      – Timo
      Apr 16 at 14:51










    • @Timo I was referring to the reverse_iterator issue with my second design point as well, so I guess you don't need to consider that anymore.
      – Ben Steffan
      Apr 16 at 15:01










    • Alright. I think I fixed everything. Can you confirm, please?
      – Timo
      Apr 16 at 15:06










    • @Timo I'm sorry, but you will have to roll back your edit. The validity and usefulness of answers on Code Review depends on code in questions not being edited after there is at least one review. If you still have doubts about your code, please post a follow up question instead.
      – Ben Steffan
      Apr 16 at 15:13






    • 1




      @yuri, for standard library it is not hard to remember (may be if you're working on a project with bizillion includes). There are off course edge cases, which for me is multithreading stuff.
      – Incomputable
      Apr 16 at 16:25















    I completely forgot about std::reverse_iterator. Thanks for pointing that out. About your second design issue: what additional functionality are you referring to? The only overhead I can see is getUnit(), which I thought would be helpful if it's accessible from outside. Other Things: 3: I thought I read on cppreference that std::ptrdiff_t is marked as deprecated in C++17, but looks like I messed that up. I will fix the other points asap.
    – Timo
    Apr 16 at 14:51




    I completely forgot about std::reverse_iterator. Thanks for pointing that out. About your second design issue: what additional functionality are you referring to? The only overhead I can see is getUnit(), which I thought would be helpful if it's accessible from outside. Other Things: 3: I thought I read on cppreference that std::ptrdiff_t is marked as deprecated in C++17, but looks like I messed that up. I will fix the other points asap.
    – Timo
    Apr 16 at 14:51












    @Timo I was referring to the reverse_iterator issue with my second design point as well, so I guess you don't need to consider that anymore.
    – Ben Steffan
    Apr 16 at 15:01




    @Timo I was referring to the reverse_iterator issue with my second design point as well, so I guess you don't need to consider that anymore.
    – Ben Steffan
    Apr 16 at 15:01












    Alright. I think I fixed everything. Can you confirm, please?
    – Timo
    Apr 16 at 15:06




    Alright. I think I fixed everything. Can you confirm, please?
    – Timo
    Apr 16 at 15:06












    @Timo I'm sorry, but you will have to roll back your edit. The validity and usefulness of answers on Code Review depends on code in questions not being edited after there is at least one review. If you still have doubts about your code, please post a follow up question instead.
    – Ben Steffan
    Apr 16 at 15:13




    @Timo I'm sorry, but you will have to roll back your edit. The validity and usefulness of answers on Code Review depends on code in questions not being edited after there is at least one review. If you still have doubts about your code, please post a follow up question instead.
    – Ben Steffan
    Apr 16 at 15:13




    1




    1




    @yuri, for standard library it is not hard to remember (may be if you're working on a project with bizillion includes). There are off course edge cases, which for me is multithreading stuff.
    – Incomputable
    Apr 16 at 16:25




    @yuri, for standard library it is not hard to remember (may be if you're working on a project with bizillion includes). There are off course edge cases, which for me is multithreading stuff.
    – Incomputable
    Apr 16 at 16:25












    up vote
    0
    down vote













    Take a look at Boost.Iterator.



    You can write what you want by a simple use of Boost’s iterator_adaptor, changing only what you need to change (e.g. advance member). Seriously, it does all that code you wrote, already in the can and well reviewed for edge cases. You only have to write what’s different from the underlying iterator, and everything else comes through.



    Or, if you’re living on the bleeding edge, just use a view::stride in the Range v3 library.




    Given a source range and an integral stride value, return a range consisting of every Nth element, starting with the first.







    share|improve this answer

























      up vote
      0
      down vote













      Take a look at Boost.Iterator.



      You can write what you want by a simple use of Boost’s iterator_adaptor, changing only what you need to change (e.g. advance member). Seriously, it does all that code you wrote, already in the can and well reviewed for edge cases. You only have to write what’s different from the underlying iterator, and everything else comes through.



      Or, if you’re living on the bleeding edge, just use a view::stride in the Range v3 library.




      Given a source range and an integral stride value, return a range consisting of every Nth element, starting with the first.







      share|improve this answer























        up vote
        0
        down vote










        up vote
        0
        down vote









        Take a look at Boost.Iterator.



        You can write what you want by a simple use of Boost’s iterator_adaptor, changing only what you need to change (e.g. advance member). Seriously, it does all that code you wrote, already in the can and well reviewed for edge cases. You only have to write what’s different from the underlying iterator, and everything else comes through.



        Or, if you’re living on the bleeding edge, just use a view::stride in the Range v3 library.




        Given a source range and an integral stride value, return a range consisting of every Nth element, starting with the first.







        share|improve this answer













        Take a look at Boost.Iterator.



        You can write what you want by a simple use of Boost’s iterator_adaptor, changing only what you need to change (e.g. advance member). Seriously, it does all that code you wrote, already in the can and well reviewed for edge cases. You only have to write what’s different from the underlying iterator, and everything else comes through.



        Or, if you’re living on the bleeding edge, just use a view::stride in the Range v3 library.




        Given a source range and an integral stride value, return a range consisting of every Nth element, starting with the first.








        share|improve this answer













        share|improve this answer



        share|improve this answer











        answered Apr 17 at 8:42









        JDługosz

        5,047731




        5,047731






















             

            draft saved


            draft discarded


























             


            draft saved


            draft discarded














            StackExchange.ready(
            function ()
            StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f192195%2fcustom-c-random-access-iterator-for-pointers-to-arrays%23new-answer', 'question_page');

            );

            Post as a guest













































































            Popular posts from this blog

            Greedy Best First Search implementation in Rust

            Function to Return a JSON Like Objects Using VBA Collections and Arrays

            C++11 CLH Lock Implementation