I made a FibonacciSeries functor for using in std::generate_n. But it doesn't work the way it should be. Passing functor by reference doesn't work well.

In the following program 0 1 1 2 3 5 8 13 is the expected output but I get 0 1 1 2 0 1 1 2 as the output. What could be wrong?

#include <iostream>
#include <vector>
#include <algorithm>
#include <iterator>

using namespace std;

inline void printvec(const vector<int>& vec)
{
    for (const auto& v : vec)
        cout << v << ' ';
    cout << endl;
}


class FibonacciSeries
{
private:
    int f1;
    int f2;
    int f3;
public:
    FibonacciSeries() :f1(0), f2(1) {}
    int operator() () {    
        f3 = f1   f2;
        int  result = f1;
        f1 = f2;
        f2 = f3;
        return result;
    }
    void printstat(){
        cout << f1 << endl;
    }
};

int main()
{
    vector<int> vec;
    FibonacciSeries series;
    //passing function object by reference
    generate_n<back_insert_iterator<vector<int>>, 
        int, FibonacciSeries&>(back_inserter(vec), 4, series);

    printvec(vec);// prints 0 1 1 2

    series.printstat();//prints 0//unexpected
    
    //passing function object by value
    generate_n(back_inserter(vec), 4, series);
    printvec(vec);//prints 0 1 1 2 0 1 1 2//unexpected
}

CodePudding user response：

I wasnt able to reproduce the issue with gcc trunk (https://godbolt.org/z/PfPPE67h8). However, standard algorithms may copy functors passed to them internally. Not sure what is actually going "wrong" in the implementation but suppose the algorithm starts with

  std::remove_reference<Generator> g = generator;

And uses that to generate the output. One would have to read the implementation to see whats causing your output. However, using FibonacciSeries& as explicit template argument is the wrong approach. Even if the generator is passed by reference it may be copied internally.

The simplest to avoid such issues as you are observing now, is to not rely on functors with state. Rather than making the state member of the functor, let it capture state by reference and store the state elsewhere. Either use a lamba, as suggested by Sam in a comment, or write your custom wrapper:

struct FibonacciSeriesWrapper {
    FibonacciSeries& parent;
    int operator() () { return parent(); }
};

int main()
{
    vector<int> vec;
    FibonacciSeries series;    
    //passing function object by reference
    generate_n(back_inserter(vec), 4, FibonacciSeriesWrapper{series});
    generate_n(back_inserter(vec), 4, FibonacciSeriesWrapper{series});
    printvec(vec);//prints 0 1 1 2 3 5 8 13
}

Live Demo

CodePudding user response：

Your generator (FibonnachiSeries object) is copied to std::generate_n call. So original object state is not changed. One workaround may be passsing a lambda

generate_n(back_inserter(vec), 4, 
           [&series] { return series(); });

// UPD: nevermind, declaring FibonacchiSeries as a reference should have helped. Your version works fine for me. What compiler do you use?