Bidirectional iterator implementation

Question

By some reasons I need to implement a bidirectional iterator, after some time, I got this result (add parameter tells what the side the iterator should move to (to avoid code duplication, when implementing reverse_iterator)):

#include <iterator>

namespace gph {
    template <typename T, int add> class BidirectionalIterator;

    template<typename T, int add>
    void swap(BidirectionalIterator<T, add>& it1, BidirectionalIterator<T, add>& it2) {
        it1.swap(it2);
    }

    template<typename T, int add>
    class BidirectionalIterator {
    private:
        T *currentPosition, *begin, *end;
    public:
        using difference_type = std::ptrdiff_t;
        using value_type = T;
        using pointer = T*;
        using reference = T&;
        using iterator_category = std::bidirectional_iterator_tag;

        inline BidirectionalIterator(T* currentPosition, T* begin, T* end):currentPosition(currentPosition), begin(begin), end(end) {}

        //copy constructor
        inline BidirectionalIterator(const BidirectionalIterator& iterator)
            :BidirectionalIterator(iterator.currentPosition, iterator.begin, iterator.end) {}

        //move constructor
        inline BidirectionalIterator(BidirectionalIterator&& iterator) noexcept
            :BidirectionalIterator(iterator.currentPosition, iterator.begin, iterator.end) {}

        //copy and move assignment statement
        inline BidirectionalIterator& operator=(BidirectionalIterator iterator) {
           gph::swap(*this, iterator);
        }

        inline void swap(BidirectionalIterator& iterator) {
            std::swap(currentPosition, iterator.currentPosition);
            std::swap(begin, iterator.begin);
            std::swap(end, iterator.end);
        }

        inline reference operator*() const {
            return *currentPosition; //dangerous if the iterator is in not-dereferenceable state
        }

        inline BidirectionalIterator& operator++() {
            if (currentPosition != end) currentPosition += add;

            return *this;
        }

        inline bool operator==(const BidirectionalIterator& iterator) const {
            return currentPosition == iterator.currentPosition;
        }

        inline bool operator!=(const BidirectionalIterator& iterator) const {
            return !(*this == iterator);
        }

        inline BidirectionalIterator operator++(int) {
            BidirectionalIterator past = *this;

            ++*this;

            return past;
        }

        inline BidirectionalIterator& operator--() {
            if (currentPosition != begin) currentPosition -= add;

            return *this;
        }

        inline BidirectionalIterator operator--(int) {
            BidirectionalIterator past = *this;

            --*this;

            return past;
        }
    };
}

I've tried to satisfy MoveAssignable, MoveConstructible, CopyAssignable, CopyConstructible, Swappable, EqualityComparable, LegacyIterator, LegacyInputIterator, LegacyForwardIterator, LegacyBidirectionalIterator named requirements.

Some of their requirements are expressed in operators overloading, but some ones from them I do not know how to implement (perhaps, they are automatically implemented by other ones?), for instance: i->m or *i++ (from here). First question: how should I implement them?

Second question: is my iterator implementation good? What are its drawbacks, where did I make mistakes?

P.S. The questions are on edge of unconstructiveness, but I really need help with it. Sorry for my english.

Answer 1

I find it hard to find a definitive answer to this, so just some thoughts, which may be uncomplete and are open to discussion.

• i->m can be implemented by inline pointer operator->() { return this->currentPosition; }
• *i++ should already be covered by your implementation

• I don't see any reason to swap all the pointers in operator=. For three reasons:

  1. You are swapping the values with a local variable
  2. The move constructor doesn't swap any values (would be inconsistent behaviour between BidirectionalIterator newIt=oldIt; and BidirectionalIterator newIt(oldIt);, but it actually isn't because of the previous point)
  3. Those pointers are not unique resources, so there is no problem in copying them and sharing them between multiple instances
• operator= is missing a return.
• You have using difference_type = std::ptrdiff_t; but don't implement operator- which would return difference_type, why not implement it?
• Reverse iterators can be implemented easier by std::reverse_iterator which will just wrap your iterator and invert ++ and -- and so on.
• You probably want to find an easy way to implement a const version of the iterator (a version that always returns a const T& or const T*). I saw three versions of this:
  • duplicating all the code
  • using const cast
  • using an additional template parameter bool TIsConst and using pointer = std::conditional_t<TIsConst, const T*, T*>;
  • using a templated iterator with parameter const T on the other hand might appear easy, but fails to satisfy a requirement, see this question