4
\$\begingroup\$

I wrote this snippet:

template <typename ContainerT>
class ReverseIterator
{
public:
    ReverseIterator(ContainerT& iContainer) : m_container{iContainer}
    {

    }

    typename ContainerT::reverse_iterator begin()
    {
        return m_container.rbegin();
    }

    typename ContainerT::reverse_iterator end()
    {
        return m_container.rend();
    }

private:
    ContainerT& m_container;
};

template <typename ContainerT>
auto Reverse(ContainerT& iContainer)
{
    return ReverseIterator<ContainerT>(iContainer);
}

To be able to do something like this:

std::vector<int> myList{1, 2, 3};
for (auto& itr : Reverse(myList))
{
    ++itr;
}

First I'd like a feedback up to this point.

Then, I'd like to be able to write something like this:

for (auto itr : Reverse(std::vector<int>{1, 2, 3}))
{
    std::cout << itr << " ";
}

Without having a separate and near identical implementation of ConstReverse.

I thought about making everything with forwarding references but for some reason that didn't work out very well and I'm not exactly sure where I was mistaken. Could someone show me how it could be done?

\$\endgroup\$
4
\$\begingroup\$

Good clear template code - well done!

I'd consider changing the name, as the class itself isn't an iterator, it's an iterator pair, or range, that wraps a container and provides a reversed view of it.

Constructor should be explicit.

We should use free-function rbegin() and rend() to give us greater flexibility of container types (e.g. C-style arrays) - fall back to the std versions in <iterator> if argument-dependent lookup fails.

It could be improved by overloading Reverse with a version that accepts a const Container:

template <typename ContainerT>
auto Reverse(const ContainerT& iContainer)
{
    return ReverseIterator<const ContainerT>(iContainer);
}

For this to work properly, we need to change the return type of begin() and end() to auto, so that we get const_reverse_iterator back from them. If you're really stuck with C++11, then we'll have to add the trailing return type: auto begin()-> decltype(m_container.begin()), for example. For my own convenience, I'll work with C++17 from here onwards.


Applying these suggested changes gives us:

template <typename ContainerT>
class ReverseContainerView
{
public:
    explicit ReverseContainerView(ContainerT& iContainer)
      : m_container{iContainer}
    {
    }

    auto begin() {
        using std::rbegin;
        return rbegin(m_container);
    }
    auto end() {
        using std::rend;
        return rend(m_container);
    }

private:
    ContainerT&  m_container;
};


template<typename ContainerT>
auto Reverse(ContainerT& iContainer)
{
    return ReverseContainerView<ContainerT>(iContainer);
}

template<typename ContainerT>
auto Reverse(const ContainerT& iContainer)
{
    return ReverseContainerView<const ContainerT>(iContainer);
}

and demo:

#include <iostream>
#include <vector>
int main()
{
    auto list = std::vector<int>{1, 2, 3};
    for (auto& i: Reverse(list)) {
        std::cout << ++i << ' ';
    }
    std::cout << '\n';
    for (const auto& i: list) {
        std::cout << i << ' ';
    }
    std::cout << '\n';

    const auto const_list = std::vector<int>{1, 2, 3};
    for (auto i: Reverse(const_list)) {
        std::cout << i << ' ';
    }
    std::cout << '\n';
}

Note that the proposed usage for (auto i: Reverse(std::vector<int>{10, 20, 30})) (where the constructor argument is an xvalue) won't work with the above code. That's because the temporary has gone out of scope after the ReverseContainer was constructed but before the for loop completes, leaving a dangling reference in the container. We'll need a version that captures and stores any rvalue type passed to it.

The easiest way I know to do this is to split the class into its two responsibilities: a base class to capture the wrapped container and a subclass to access the reversed view (we might want to put some or all of these classes into an inner namespace to hide them from users; I'll leave that question open here).

#include <iterator>
#include <type_traits>
#include <utility>

template <typename T, bool CopyValue = !std::is_lvalue_reference_v<T>>
struct ContainerContainer;

template <typename T>
struct ContainerContainer<T, false>
{
    // lvalue version: contains a reference to the container
    T& container;
    explicit ContainerContainer(T& container)
      : container{container}
    {}
};

template <typename T>
struct ContainerContainer<T, true>
{
    // rvalue version: move-constructs a new copy of container
    const T container;
    explicit ContainerContainer(T container)
      : container{std::move(container)}
    {}
};
template <typename Container>
class ReverseContainerView : ContainerContainer<Container>
{
    using Base = ContainerContainer<Container>;
public:
    explicit ReverseContainerView(Container&& container)
        : Base{std::forward<Container>(container)}
    {}

    auto begin() {
        using std::rbegin;
        return rbegin(Base::container);
    }
    auto end() {
        using std::rend;
        return rend(Base::container);
    }
};


template<typename Container>
auto Reverse(Container&& container)
{
    return ReverseContainerView<Container>(std::forward<Container>(container));
}
// test program
#include <iostream>
#include <vector>
int main()
{
    int list[] = {1, 2, 3};
    for (auto& i: Reverse(list)) {
        std::cout << ++i << ' ';
    }
    std::cout << '\n';

    const auto const_list = {1, 2, 3}; // initializer_list
    for (auto& i: Reverse(const_list)) {
        // ++i; ERROR: 'i' is read-only
        std::cout << i << ' ';
    }
    std::cout << '\n';

    for (auto& i: Reverse(std::vector<int>{10, 20, 30})) {
        // ++i; ERROR: 'i' is read-only
        std::cout << i << ' ';
    }
    std::cout << '\n';
}

From C++17 onwards, we don't really need the Reverse() helper function: we can instead simply provide a template deduction guide:

template<typename Container>
explicit ReverseContainerView(Container&& container)
    -> ReverseContainerView<Container>;

We might want to choose a shorter name for the class now that it's more visible to the users.

\$\endgroup\$
  • \$\begingroup\$ excellent improvements! I have a question though, why didn't we capture/store by rvalue reference in the ContainerContainer (the rvalue variant of it) ? \$\endgroup\$ – Tedy S. Oct 24 '18 at 11:15
  • \$\begingroup\$ If we're given an rvalue reference, we need to copy it. If we just keep a reference to a temporary value, the reference will be invalid once the value is destructed (and that's before the for loop calls begin()). If you (or anyone) has suggestions on how I can improve the sentence where I explain that, please add a comment or make a suggested edit. \$\endgroup\$ – Toby Speight Oct 24 '18 at 11:35
  • \$\begingroup\$ indeed it makes sense. how about a const ref, shouldn't it prolong the lifetime of the temporary? I know it doesn't make sense to keep it as a const ref in this context but just for the sake of trying I think I did that the other day and I was still left with a dangling ref \$\endgroup\$ – Tedy S. Oct 24 '18 at 11:51
  • 1
    \$\begingroup\$ Const ref doesn't work, because the lifetime extension isn't transitive. See Does a const reference class member prolong the life of a temporary? over on Stack Overflow. \$\endgroup\$ – Toby Speight Oct 24 '18 at 12:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.