vector push_back shows absurd result

Question

I am trying to make a vector out of several integers by overloading the operator << and conversion operator. However, when I test my code, I observe some absurd results.

The printed output should be 1 2 3 4.

But it actually print out something like this: 28495936 0 3 4.

The first two elements (e.g, 1 and 2) that were supposed to be pushed into the vector is lost or polluted.

I would appreciate it if someone can help me to figure it out the reason behind this.

#include <iostream>
#include <vector>

using namespace std;

template<typename T>
class make_vector {
public:
    typedef make_vector<T> my_type;
    my_type& operator<<(const T& val)
    {
        data_.push_back(val);
        return *this;
    }
    operator std::vector<T>&()
    {
        return  this->data_;
    }

private:
    std::vector<T> data_;
};


int main() {
    std::vector<int>&  A2 = make_vector<int>() << 1 << 2 << 3 << 4;

    for (std::vector<int>::iterator it = A2.begin(); it != A2.end(); ++it)        
    {
        cout << *it << " ";
    }
    cout << endl;
    return 0;
}

juanchopanza · Answer 1 · 2015-02-09T19:38:08.727

3

You are binding a lvalue reference to a temporary:

std::vector<int>&  A2 = make_vector<int>() << 1 << 2 << 3 << 4;

Note that, as @T.C. mentions in comments, it is your conversion operator that enables this. Without it the statement above would be invalid C++.

The issue with your code is that after this line, A2 refers to a defunct object.

It seems to me you don't want to use a reference:

std::vector<int>  A2 = make_vector<int>() << 1 << 2 << 3 << 4;

edited Feb 09 '15 at 19:38

answered Feb 08 '15 at 23:52

juanchopanza

223,364
34
402
480

I see the problem now. It helps! Thank you for your clarification. – swm Feb 09 '15 at 00:35
You don't need an extension for this to compile, thanks to the poorly designed `operator std::vector&()`. It should really be ref-qualified; and `operator<<` should probably also have two ref-qualified overloads so that you preserve the value category. – T.C. Feb 09 '15 at 19:31
@T.C. I had completely missed that. Thanks! – juanchopanza Feb 09 '15 at 19:38

Drew Dormann · Answer 2 · 2015-02-09T19:42:07.233

1

You have a dangling reference here. It references a temporary.

std::vector<int>&  A2 = make_vector<int>()....

You have two options:

You can copy the temp into a new, local variable.

std::vector<int>  A2 = make_vector<int>()....

~~Or use a const ref. C++ grants a special rule to allow const references to extend the lifetime of a temporary.~~

const std::vector<int>&  A2 = make_vector<int>()....

edited Feb 09 '15 at 19:42

answered Feb 08 '15 at 23:52

Drew Dormann

59,987
13
123
180

1

const ref won't work. You are binding to the return value of a member function, rather than the temporary directly; that doesn't extend the lifetime. – T.C. Feb 09 '15 at 19:36
@T.C. I never knew that. Thanks for the tip! – Drew Dormann Feb 09 '15 at 19:43

score 0 · Answer 3 · answered Feb 09 '15 at 19:51

Your make_vector is badly designed and dangerous to use. You made it convertible to std::vector<T>& referring to its vector member, even if the make_vector object is a temporary. This is a dangling reference waiting to happen, since with this setup the compiler won't complain when you do things like

std::vector<int>&  A2 = make_vector<int>() << 1 << 2 << 3 << 4;

because everything in this line is perfectly valid. Except that it leaves A2 a dangling reference as the make_vector temporary - and the vector it contains - gets destroyed at the ;. Ouch.

A better design would ref-qualify and overload your operator<< and the conversion function:

my_type& operator<<(const T& val) &
{
    data_.push_back(val);
    return *this;
}
my_type&& operator<<(const T& val) &&
{
    data_.push_back(val);
    return std::move(*this);
}

operator std::vector<T>&() &
{
    return  this->data_;
}

operator std::vector<T>() &&
{
    return  std::move(this->data_);
}

First, we make only lvalue make_vectors convertible to an lvalue reference to the underlying vector, since this is probably safe. For rvalue make_vectors, such as temporaries, we make the conversion function return the vector by value by moving from the underlying vector.

Second, we overload operator<< to preserve the value category of the make_vector object it is invoked on - it returns an lvalue reference if invoked on an lvalue, and an rvalue reference if invoked on an rvalue. This way, make_vector<int>() << 1 << 2 << 3 << 4 remains an rvalue.

Now errors like std::vector<int>& A2 = make_vector<int>() << 1 << 2 << 3 << 4; won't compile, while both

std::vector<int> A2 = make_vector<int>() << 1 << 2 << 3 << 4;

and

const std::vector<int> & A2 = make_vector<int>() << 1 << 2 << 3 << 4;

will and both are safe. (In the second case, the const reference binds to the temporary returned by operator std::vector<int>(), which extends the lifetime of the temporary.)

T.C., I really appreciate your answer. There is enough information for me to digest and learn now. I will make sure that I can vote this answer up after I gain enough reputation later. — swm, Feb 09 '15 at 20:36
should the first conversion function be changed to operator std::vector() & ? — swm, Feb 09 '15 at 23:59
@user1487243 Not needed; the point is that when the object is an lvalue, returning a reference is safe. — T.C., Feb 10 '15 at 04:13

vector push_back shows absurd result

3 Answers3