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 !

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.