2
\$\begingroup\$

I have created a little generator class to generate number sequences.The intended purpose of this class is to be used with generate_n to populate containers. I would like to know whether there is something which i can do to improve performance or usability or to make the code more generic. It should be possible to compile the code with a c++14 conformant compiler at the moment, but i am open to use any number of c++17 features. My generator class:

#include <functional>

template<typename Number,
         typename AdvanceOperation = std::plus<Number>>
class sequence_generator
{
  public:
    constexpr sequence_generator(Number start_value = Number{},
                                 Number step_size = Number{1},
                                 AdvanceOperation advancer = AdvanceOperation{})
                                 : current_value{start_value},
                                   step_size{step_size},
                                   advancer{advancer}
    {}
    constexpr auto operator()() -> Number
    {
      return next();
    }
    constexpr auto next() -> Number
    {
      auto tmp = current_value;
      current_value = advancer(current_value, step_size);
      return tmp;
    }
  private:
    Number current_value;
    Number step_size;
    AdvanceOperation advancer;
};

template<typename Number, typename AdvanceOperation=std::plus<Number>>
auto make_sequence_generator(Number start_value, Number step_size,
                             AdvanceOperation advancer)
{
    return sequence_generator<Number, AdvanceOperation>{start_value, step_size,
                                                        advancer};
}

Some use cases:

#include <algorithm>
#include <iostream>
#include <vector>
#include <cassert>

#include "generator.hpp"

int main()
{
  std::vector<int> vector(10);
  std::generate(std::begin(vector), std::end(vector),
                sequence_generator<int>{});
  assert(vector == std::vector<int>({0,1,2,3,4,5,6,7,8,9}));
  std::generate(std::begin(vector), std::end(vector),
                make_sequence_generator(1,2,std::multiplies<>{}));
  assert(vector == std::vector<int>({1,2,4,8,16,32,64,128,256,512}));
}
\$\endgroup\$
  • \$\begingroup\$ have you considered functionality of std::iota? I think it has nice interface. \$\endgroup\$ – Incomputable Jul 27 '16 at 10:34
  • \$\begingroup\$ @OlzhasZhumabek Didn't knew that such a function existed. I thought about a freestanding function to ease the process, and std::iota is a great example, although with limited functionality. \$\endgroup\$ – Vincent Jul 27 '16 at 11:01
1
\$\begingroup\$

Generally code looks good. Despite that, there are number of places where it could be improved.

There is no need to specify output type explicitly. I'm not a language lawyer, but return type seems explicit to me. Also, ideone compiler is able to compile it without return type hints.

The behavior of the next is surprising. It returns old value and then advances it. I'd expect it to be vice versa. I guess you wanted to output initial value once, so it is probably a design choice. On top of that, it is probably the most performance draining part of the code.

There is no need to write constexpr on every function. It will mean inline, which itself means bloating size of the executable. Cases where it could be really compile time evaluated are very rare. The deal breaker factor here is advancer function, and if it returns different values on each call it is probably impossible to evaluate it at compile time. I'm excluding possibilities of immense template metaprogramming magic.

I would change interface to use binary operation to advance it. It would be much more customizable.

Overall I think std::generate_n is more for randomly generated numbers or something alike. People usually use std::iota to fill with increasing values.

Proposal

I propose a very different solution.

The solution will be a generic function named stepping_iota.

template <typename OutputIterator, typename T, typename UnaryAdavanceOp>
void stepping_iota(OutputIterator first, OutputIterator last, T init_value, UnaryAdavanceOp op)

It will call std::iota under the hood, to avoid surprises. But to do that, it will use proxy pattern. Specifications say that it is not required for decltype(*first) and T to match. So we can implement very small proxy, which will have all the needed functionality.

namespace impl_details
{
    template <typename T, typename UnaryAdvanceOp>
    class proxy
    {
        T value;
        UnaryAdvanceOp op;
    public:
        proxy(T init_value, UnaryAdvanceOp op_) :
            value(init_value),
            op(op_)
        {}

        operator T()
        {
            return value;
        }

        T operator++()
        {
            value = op(value);
            return value;
        }

        T operator++(int)
        {
            T old = value;
            value = op(value);
            return old;
        }
    };
}

I've written post increment for the sake of symmetry.

Then the stepping_iota will construct the proxy and pass it to std::iota:

impl_details::proxy<T, UnaryAdavanceOp> p(init_value, op);
std::iota(first, last, p);

Despite the function has a few parameters, it is customizable and easy to use with lambdas. It uses unary operation, which is common and used in functions such as std::transform.

Complete example:

#include <numeric>

namespace impl_details
{
    template <typename T, typename UnaryAdvanceOp>
    class proxy
    {
        T value;
        UnaryAdvanceOp op;
    public:
        proxy(T init_value, UnaryAdvanceOp op_) :
            value(init_value),
            op(op_)
        {

        }

        operator T()
        {
            return value;
        }

        T operator++()
        {
            value = op(value);
            return value;
        }

        T operator++(int)
        {
            T old = value;
            value = op(value);
            return old;
        }
    };
}

template <typename OutputIterator, typename T, typename UnaryAdavanceOp>
void stepping_iota(OutputIterator first, OutputIterator last, T init_value, UnaryAdavanceOp op)
{
    impl_details::proxy<T, UnaryAdavanceOp> p(init_value, op);
    std::iota(first, last, p);
}

#include <vector>
#include <iostream>

int main()
{
    std::vector<int> v(10'000);


    stepping_iota(v.begin(), v.end(), 0, [](const int& x) { return x + 1; });

    for (const auto& elem : v)
    {
        std::cout << elem << ' ';
    }
}

In C++17 you will be able to write:

impl_details::proxy p( init_value, op );
\$\endgroup\$
  • \$\begingroup\$ Thank you for your review. I think, that your Solution is very elegant and generic. It also builds on a standard library function, which is very nice. \$\endgroup\$ – Vincent Jul 30 '16 at 9:24
  • \$\begingroup\$ @Vincent, I believe there is a possibility to make a more generic iota and easy to use, which will fall back to simple iota. The problem here is that template parameters doesn't support lambdas as default parameters due to runtime nature of them. May be in the future it will be possible. Probably the name will need to be changed though \$\endgroup\$ – Incomputable Jul 30 '16 at 12:43

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.