Const wrapper for iterator

Question

Edit: added clarification for why I want this, and updated the code since I don't have any answers yet

I have a C++11 array-like class which (can be) a wrapper around a random-access iterator. Index-based access and .begin()/.end() can just pass through to the iterator, but there's a bit of complication when the object is const:

    template<class Size, typename DataIterator>
    class Storage : public Size {
        DataIterator iterator;
    public:
        Storage(const DataIterator &iterator, const Size &size) : Size(size), iterator(iterator) {}
        
        auto operator[](index_t i)
                -> decltype(iterator[i]) {
            return iterator[i];
        }
        auto operator[](index_t i) const
                -> MakeConst<decltype(iterator[i])> {
            return iterator[i];
        }
        
        DataIterator begin() {return iterator;}
        ConstWrapper<DataIterator> begin() const {return iterator;}
        DataIterator end() {return iterator + this->size();}
        ConstWrapper<DataIterator> end() const {return iterator + this->size();}
    };

If we simply returned DataIterator from the const version of .begin() and .end(), then people holding a Storage const & would (incorrectly!) be able to modify the array through that iterator.

Containers like std::vector have two separate iterators (::iterator and ::const_iterator), but our Storage class only has the read-write one, so we synthesise one using ConstWrapper:

    template <typename Iterator>
    class ConstWrapper {
        Iterator iterator;

        using traits = std::iterator_traits<Iterator>;
    public:
        using difference_type = typename traits::difference_type;
        using value_type = typename traits::value_type;
        using pointer = ConstWrapper;
        using reference = MakeConst<typename traits::reference>;
        using iterator_category = typename traits::iterator_category;
    
        ConstWrapper() {}
        ConstWrapper(const Iterator &iterator) : iterator(iterator) {}
        
        // The problematic cases:
        auto operator[](index_t i) const
                -> MakeConst<decltype(iterator[i])> {
            return iterator[i];
        }
        auto operator*() const
                -> MakeConst<decltype(*iterator)> {
            return *iterator;
        }

        bool operator!= (const ConstWrapper& other) const {
            return iterator != other.iterator;
        }
        /** All the other random-access iterator methods **/
    };
    
    // Specialisation to prevent infinite loops
    template <typename Iterator>
    class ConstWrapper<ConstWrapper<Iterator>> : ConstWrapper<Iterator> {
    public:
        using ConstWrapper<Iterator>::ConstWrapper;
    };

The implementation forwards all the relevant methods, and has a specialisation for ConstWrapper<ConstWrapper<...>> so that it can't wrap itself.

A key part is MakeConst, which turns types into the correct const variant (unlike simply adding const, which has no effect on references):

    // Converts (T & -> T const &), and (T -> const T)
    using MakeConst = typename std::conditional<
        std::is_reference<T>::value,
        typename std::remove_reference<T>::type const &,
        const T
    >::type;

Does this make sense, and is it OK? Is there something else I could be doing which is more readable/efficient/etc.?

Thanks!

Answer 1

I don't think your specialization is doing what you think it's doing. https://godbolt.org/z/Gfh7sv

You don't want ConstWrapper<ConstWrapper<Iterator>> to inherit from ConstWrapper<Iterator>; that would mean that a CW<CW<I>> "is-a-kind-of" CW<I>, which isn't true. You don't want an inheritance relationship here. What you want is simply for specializations of Storage to not wrap DataIterators that happen to be specializations of ConstWrapper already. The way you do that is with an extra layer of alias indirection:

template<class T> struct maybe_constwrap { using type = ConstWrapper<T>; };
template<class U> struct maybe_constwrap<ConstWrapper<U>> { using type = ConstWrapper<U>; };

template<class Size, typename DataIterator>
class Storage : public Size {
public:
    using iterator = DataIterator;
    using const_iterator = typename maybe_constwrap<DataIterator>::type;
    ~~~
};

Now, if DataIterator is already a ConstWrapper, then both iterator and const_iterator will be literally the same type... which is what you want.

---

Your MakeConst seems to be predicated on the dubious idea that const _Bit_reference is not going to be assignable-to. That's likely to change in C++23. See https://stackoverflow.com/questions/63412623/should-c20-stdrangessort-not-need-to-support-stdvectorbool for some context.

---

You repeat some metaprogramming in the return types of operator* and operator[]. They should just return the type reference which you have already computed above.

reference operator*() const { return *it_; }

Incidentally, I'm changing your data member's name from iterator to it_, because I suspect it is a terrible idea, Concepts-wise, to have a container-ish class type with a member named iterator which member is (A) not public, and (B) not a type.

---

Your operator!= should probably use the hidden friend idiom:

friend bool operator==(ConstWrapper a, ConstWrapper b) { return a.it_ == b.it_; }
friend bool operator!=(ConstWrapper a, ConstWrapper b) { return a.it_ != b.it_; }

In C++20 you could theoretically omit operator!= and provide operator== only. I don't know yet if that's a good idea.

Passing ConstWrapper by value should be fine, because it holds only an iterator, and iterators are cheap to copy.