Random Access Iterator Implementation

Question

(This is the Non-const version, I have to implement the const one too). Could someone please review this implementation? This is made for std::vector I'm unsure whether I respected all requirements for LegacyRandomAccessIterator; so if I'm missing something, please do let me know.

namespace random_access
{
 template<typename Type>
 class iterator
 {
 private:
 Type* m_iterator;
 public:
 using value_type = Type;
 using reference = value_type&;
 using pointer = value_type*;
 using iterator_category = std::random_access_iterator_tag;
 using difference_type = std::ptrdiff_t;
 //using iterator_concept = std::contiguous_iterator_tag;
 constexpr iterator(Type* iter = nullptr) : m_iterator{ iter } {}
 constexpr bool operator==(const iterator& other) const noexcept { return m_iterator == other.m_iterator; }
 constexpr bool operator!=(const iterator& other) const noexcept { return m_iterator != other.m_iterator; }
 constexpr reference operator*() const noexcept { return *m_iterator; }
 constexpr pointer operator->() const noexcept { return m_iterator; }
 constexpr iterator& operator++() noexcept { ++m_iterator; return *this; }
 constexpr iterator operator++(int) noexcept { iterator tmp(*this); ++(*this); return tmp; }
 constexpr iterator& operator--() noexcept { --m_iterator; return *this; }
 constexpr iterator operator--(int) noexcept { iterator tmp(*this); --(*this); return tmp; }
 constexpr iterator& operator+=(const difference_type other) noexcept { m_iterator += other; return *this; }
 constexpr iterator& operator-=(const difference_type other) noexcept { m_iterator -= other; return *this; }
 constexpr iterator operator+(const difference_type other) const noexcept { return iterator(m_iterator + other); }
 constexpr iterator operator-(const difference_type other) const noexcept { return iterator(m_iterator - other); }
 constexpr iterator operator+(const iterator& other) const noexcept { return iterator(*this + other.m_iterator); }
 constexpr difference_type operator-(const iterator& other) const noexcept { return std::distance(m_iterator, other.m_iterator); }
 constexpr reference operator[](std::size_t index) const { return m_iterator[index]; }
 constexpr bool operator<(const iterator& other) const noexcept { return m_iterator < other.m_iterator; }
 constexpr bool operator>(const iterator& other) const noexcept { return m_iterator > other.m_iterator; }
 constexpr bool operator<=(const iterator& other) const noexcept { return m_iterator <= other.m_iterator; }
 constexpr bool operator>=(const iterator& other) const noexcept { return m_iterator >= other.m_iterator; }
 };
}

Thanks !

• slepic: Can I ask what is the purpose of this class?
• Loki Astari: A pointer already implements the "Random Access Iterator" concept. You don't need to wrap it in a class.
• Loki Astari: What does it mean to add two iterators together? iterator operator+(const iterator& other) That does not seem to have any meaning.

Answer 1

As pointed out by @Martin York in the comments that your class is just a wrapper for a pointer, I don't have much to say. I only have one suggestion

• Better Formatting

Good formatting IMO plays a major role in how readable your code is. In this case, I find it impossible to navigate through the functions because they all look extremely cramped.

constexpr pointer operator->() const noexcept { return m_iterator; }
constexpr iterator& operator++() noexcept { ++m_iterator; return *this; }

Compare that to

constexpr pointer operator->() const noexcept { 
 return m_iterator;
}
constexpr iterator& operator++() noexcept { 
 ++m_iterator; 
 return *this; 
}

Answer 2

I know this is late, but at least on MSVC, this is incorrect:

constexpr difference_type operator-(const iterator& other) const noexcept { 
 return std::distance(m_iterator, other.m_iterator); 
}

Internally, MSVC will attempt to do the following with begin and end iterator construction with a std::vector:

template <class _Iter, enable_if_t<_Is_iterator_v<_Iter>, int> = 0>
_CONSTEXPR20 vector(_Iter _First, _Iter _Last, const _Alloc& _Al = _Alloc())
 : _Mypair(_One_then_variadic_args_t{}, _Al) {
 _Adl_verify_range(_First, _Last);
 auto _UFirst = _Get_unwrapped(_First);
 auto _ULast = _Get_unwrapped(_Last);
 if constexpr (_Is_cpp17_fwd_iter_v<_Iter>) {
 const auto _Length = static_cast<size_t>(_STD distance(_UFirst, _ULast));
 const auto _Count = _Convert_size<size_type>(_Length);
 _Construct_n(_Count, _STD move(_UFirst), _STD move(_ULast));
// more code that isn't relevant below

When it gets to

const auto _Length = static_cast<size_t>(_STD distance(_UFirst, _ULast));

MSVC will then also attempt to call std::distance on your iterator types:

template <class _InIt>
_NODISCARD _CONSTEXPR17 _Iter_diff_t<_InIt> distance(_InIt _First, _InIt _Last) {
 if constexpr (_Is_ranges_random_iter_v<_InIt>) {
 return _Last - _First; // assume the iterator will do debug checking
 } 
 // more code that isn't relevant below

Which calls your subtraction operator on iterators on begin and end. So that which ends up swapping end and begin to call:

 return std::distance(end.m_iterator, begin.m_iterator); 

which also ends up swapping end and begin again (which are now pointers and not iterators) resulting in:

return _Last - _First; //which is now effectively 
return begin - end;

Instead, you should simply do a normal subtraction between your m_iterator and other.m_iterator.

constexpr difference_type operator-(const iterator& other) const noexcept { 
 return m_iterator - other.m_iterator; 
}

which will cause your iterator to behave normally.