Preprocessing iterator

Question

After tweaking semantics a lot, it ended up being a very poor object version of std::transform<>() that can be used with other std algorithms.

Basically the iterator wraps a provided iterator, creating an output iterator and applies supplied functor when dereferenced and assigned to. The caveat is that users will need to identify parameter type themselves, which can be error prone, but might open opportunities for other usages. Input type should not be cv qualified, because it has overloads for both const lvalue reference and rvalue reference. If the user wishes to take parameter by value, they would need to write another functor for that.

#ifndef SUNRISE_PREPROCESSING_ITERATOR_HPP
#define SUNRISE_PREPROCESSING_ITERATOR_HPP

#include <iterator>

namespace shino
{
    template<typename Functor, typename InputType, typename Iterator>
    class preprocessing_iterator :
            public std::iterator<std::output_iterator_tag,
                    InputType, void, void, void>
    {
        Functor functor;

        Iterator iterator;
    public:
        preprocessing_iterator(const Functor &f, const Iterator &it) :
                functor(f),
                iterator(it)
        {}

        preprocessing_iterator(Functor &&f, Iterator &&it) :
                functor(f),
                iterator(it)
        {}

        class proxy
        {
            friend class preprocessing_iterator;

            Iterator &iterator;
            Functor &f;
        public:
            using value_type = InputType;

            proxy &operator=(const value_type &value)
            {
                *iterator = f(value);
                return *this;
            }

            proxy &operator=(value_type &&value)
            {
                *iterator = f(value);
                return *this;
            }

        private:
            proxy(Iterator &it, Functor &functor) :
                    iterator(it),
                    f(functor)
            {}
        };

        proxy operator*()
        {
            return proxy(iterator, functor);
        }

        preprocessing_iterator &operator++()
        {
            ++iterator;
            return *this;
        }

        preprocessing_iterator operator++(int)
        {
            auto copy = *this;
            ++iterator; //might exhibit different behavior sometimes
            return copy;
        }

        const Iterator& internal_iterator() const
        {
            return iterator;
        }

        const Functor& internal_functor() const
        {
            return functor;
        }

        void swap(preprocessing_iterator& other)
        {
            using std::swap;
            swap(other.functor, functor);
            swap(other.iterator, iterator);
        }
    };

    template<typename Functor, typename InputType, typename Iterator>
    bool operator==(const preprocessing_iterator<Functor, InputType, Iterator>& lhs,
                   const preprocessing_iterator<Functor, InputType, Iterator>& rhs)
    {
        return lhs.internal_iterator() == rhs.internal_iterator();
    };

    template <typename Functor, typename InputType, typename Iterator>
    bool operator!=(const preprocessing_iterator<Functor, InputType, Iterator>& lhs,
                    const preprocessing_iterator<Functor, InputType, Iterator>& rhs)
    {
        return !(lhs == rhs);
    };
}

namespace std
{
    template <typename Functor, typename InputType, typename Iterator>
    void swap(shino::preprocessing_iterator<Functor, InputType, Iterator>& lhs,
              shino::preprocessing_iterator<Functor, InputType, Iterator>& rhs)
    {
        lhs.swap(rhs);
    };
}



#endif //SUNRISE_PREPROCESSING_ITERATOR_HPP

Usage:

#include <vector>
#include <string>
#include <algorithm>
#include <iostream>

void convert_strings_to_ints()
{
    std::vector<std::string> v{"23", "25", "27"}; //just random numbers
    std::vector<int> output(v.size());

    auto string_to_int = [](const std::string& x)
    {
        return std::stoi(x);
    };

    shino::preprocessing_iterator<decltype(string_to_int),
            std::string, std::vector<int>::iterator> conversion_iterator(string_to_int, output.begin());

    std::copy(v.begin(), v.end(), conversion_iterator);

    std::cout << "Output after converting v to vector of ints\n";

    for (const auto& element: output)
    {
        std::cout << element << '\n';
    }
}

int main()
{
    convert_strings_to_ints();
}

As it can be seen, the best usage case is writing adapters. I've tried to lower the overhead as much as possible, but if Functor is not lightweight to copy, it might become a problem if iterators are copied too much.

My specific concerns are:

• Easy of use. This is the main issue.

• Obscure places that should be documented. From the first sight of my familiar nothing seems strange or ambiguous.

• Any place where I could accidentally cause very big slow down.

Answer 1

If I understand correctly, your goal is to make this example code:

shino::preprocessing_iterator<decltype(string_to_int), std::string, std::vector<int>::iterator>
    conversion_iterator(string_to_int, output.begin());

std::copy(v.begin(), v.end(), conversion_iterator);

have an effect equivalent to this present-day code:

std::transform(v.begin(), v.end(), output.begin(), string_to_int);

(When I say it that way, it seems almost a little silly, doesn't it? ;))

---

The first usability improvement I'd make is to notice that your iterator type is fundamentally related to std::transform (actually you already noticed this) and change its name to shino::transform_iterator.

It also looks analogous to std::back_insert_iterator, in the sense that it holds references to some other objects and its constructor is really awkward to type out by hand. So I'd follow the practice of the STL here by creating a helper function. The logical name would be shino::make_transform_iterator, but you could instead follow traditional practice (cf. back_inserter) and call it simply std::transformer. It looks like this:

template<class InputType, class OutputIterator, class F>
auto transformer(F f, OutputIterator dest)
    -> transform_iterator<F, InputType, OutputIterator>
{
    return {std::move(f), std::move(dest)};
}

So now your target example code looks like this:

auto conversion_iterator = shino::transformer<std::string>(string_to_int, output.begin());
std::copy(v.begin(), v.end(), conversion_iterator);

That's an improvement!

---

Next, I notice that class InputType feels out-of-place. We don't actually use it for anything. So I'd try to get rid of it, by hunting down each place that your code depends on InputType and rewriting it to not use InputType.

For example, instead of

proxy &operator=(const value_type &value)
{
    *iterator = f(value);
    return *this;
}

proxy &operator=(value_type &&value)
{
    *iterator = f(value);
    return *this;
}

I would try to write simply

template<class U>
proxy& operator=(U&& value)
{
    *iterator = f(std::forward<U>(value));
    return *this;
}

I was a little worried that this might do the wrong thing when chaining transform_iterators together, but my first trivial test case worked, so I'm going to ignore that nagging worry. :) That test case was:

auto p1 = shino::transformer<std::string>(string_to_int, output.begin());
auto p2 = shino::transformer<std::string>([](std::string x){ return x+"1"; }, p1);
std::copy(v.begin(), v.end(), p2);

---

The other place where you use InputType is in your inheritance from std::iterator<std::output_iterator_tag, InputType, void, void, void>. Fortunately, inheritance from std::iterator is no longer considered à la mode; in fact, std::iterator itself may become deprecated soon! So what you should do is simply

template<typename Functor, typename Iterator>
class transform_iterator
{
    Functor functor;
    Iterator iterator;

  public:
    using iterator_category = std::output_iterator_tag;
    using difference_type = void;
    using value_type = void;
    using pointer = void;
    using reference = void;

Notice that std::iterator_traits strongly encourages you to put the using value_type = void; line; and pointer, reference, and difference_type also become void by analogy to std::back_insert_iterator.

Notice that these member typedefs must be public. (libc++ is happy if they're private; but libstdc++ isn't.)

---

Lastly, your specialization of std::swap is also no longer à la mode. Instead of interjecting your own code into the std namespace, you should simply provide a shino::swap(transform_iterator&, transform_iterator&) that can be found by ADL.

Your swap code itself is 100% correct and reasonable; you should just change that namespace std to namespace shino, and you'll be in business.

The expected way for users (such as STL algorithms) to swap two transform_iterators a and b is:

using std::swap;  // in case no ADL swap exists
swap(a, b);  // look for an ADL swap; fall back on std::swap if needed

---

I have no particular comment on the efficiency or subtle correctness of how your code (or my revised code) handles the forwarding of functors and such. I'd probably just try to write some tests for that (e.g. what if the functor in question is move-only?), and fix any issues they find, and then move on with my life. ;)

---

Here's a Wandbox of the code after these changes.