Using default value for const reference argument causes crash

Question

I have some code like this:

struct Data {
    Data(const std::vector<int> &data = {}) : data_(data) {}
    const std::vector<int> &data_;
};

Data create1() {
    return Data(); // bad
}

Data create2() {
    return Data({}); // bad
}

Data create3(const std::vector<int> &data = {}) {
    return Data(data); // good
}

Data create4() {
    static const std::vector<int> data;
    return Data(data); // good
}

void main() {
    auto data1 = create1(); // deleted data_
    auto data2 = create2(); // deleted data_
    auto data3 = create3(); // ok
    auto data4 = create4(); // ok
}

The four create functions seem the same to me. But why do create1 and create2 cause a deleted data_ but create3 and create4 are fine?

You're storing a reference to a temporary. That's a no-no. Change your constructor to `Data(std::vector& data);` to avoid this. — alter_igel, Jul 05 '18 at 23:10
Illegal in Standard C++, @scohe001 , but I'm pretty sure you knew that. — user4581301, Jul 05 '18 at 23:12
@alter igel But why isn't create3 considered as temp object? — Fan, Jul 05 '18 at 23:32

score 1 · Answer 1 · answered Jul 05 '18 at 23:33

1

Create static Data member and use it for initialization:

struct Data {
    Data(const std::vector<int> &data = empty_) : data_(data) {}
    const std::vector<int> &data_;
private:
    static std::vector<int> empty_;
};

std::vector<int> Data::empty_ = {};

Data create1() {
    return Data(); // good
}

Data create2() {
    return Data({}); // still bad, as it should be
}

Data create3(const std::vector<int> &data = {}) {
    return Data(data); // good
}

Data create4() {
    static const std::vector<int> data;
    return Data(data); // good
}

void main() {
    auto data1 = create1(); // ok
    auto data2 = create2(); // deleted data_
    auto data3 = create3(); // ok
    auto data4 = create4(); // ok
}

answered Jul 05 '18 at 23:33

Michael Nastenko

2,785
1
10
14

Could you explain why default value in create3 works but not in the constructor? – Fan Jul 05 '18 at 23:35
1

@Fan I don't think it is OK. I'm pretty sure dies one step later. – user4581301 Jul 05 '18 at 23:47
@user4581301 What confuses me the most is that create3 always works. Probably it is related to this: https://stackoverflow.com/questions/2784262/does-a-const-reference-prolong-the-life-of-a-temporary, and {} in create3 is stored locally in main() – Fan Jul 05 '18 at 23:57
@Fan couldn't explain what went wrong in a comment, so I've posted a semi-answer. – user4581301 Jul 06 '18 at 00:10

score 1 · Accepted Answer · answered Jul 06 '18 at 00:08

The first two cases are pretty clear abuse of temporaries. Based on the asker's comments they've gotten what went wrong in create1 and create2 at this point, so let's focus on create3 and why it works.

Spoiler: It doesn't.

I'm going to take a few liberties with the code to make what's happening a bit more obvious. First, we replace vector with a simple class that lets us see construction and destruction better.

struct test
{
    test()
    {
        std::cout << "test ctor\n";
    }
    ~test()
    {
        std::cout << "test dtor\n";
    }
};

Now we do something similar to Data and make it use test instead of vector

struct Data {
    Data(const test &data = {}) : data_(data) 
    {
        std::cout << "data ctor\n";
    }
    ~Data()
    {
        std::cout << "data dtor\n";
    }
    const test &data_;

    void forceuse() // just to make sure the compiler doesn't get any bright ideas about 
                    // optimizing Data out before we're through with it.
    {
        std::cout << "Using data\n";
    }
};

And we add a bit of extra diagnostic to create3 and once again replace vector with test

Data create3(const test &data = {}) {
    std::cout << "in create3\n";
    return Data(data); // good
}

and do the same to main

int main() {
    { // change the scope of data3 so that we can put a breakpoint at the end of main 
      // and watch all of the fun
        std::cout << "calling create3\n";
        auto data3 = create3(); // ok
        std::cout << "returned from create3\n";
        data3.forceuse();
    }
}

Output of this is

calling create3
test ctor
in create3
data ctor
test dtor
returned from create3
Using data
data dtor

test is created during the call to create3 and destroyed on exit from create3. It is not alive in main. If it seems to be alive in main, that's just dumb bad luck. Your friend and mine Undefined Behaviour being a jerk. Again.

test is created before Data, but also destroyed before Data leaving Data in a bad state.

Here's the above code all nicely assembled and run in an online IDE: https://ideone.com/XY30XH

I thought the vector would set the internal pointer to null or something when it is destroyed in debug mode but it does not seem to work in that way. So even in debug mode a destroyed vector can still work well. Now I know a default argument is not static. — Fan, Jul 06 '18 at 02:10
In C++ it's best to assume the worst and expect no hand-holding you don't expressly ask for, even in debug mode. Personally I dislike it when the debugger changes the behaviour to make some bugs more visible because it hides other bugs in the process. — user4581301, Jul 06 '18 at 04:19

Using default value for const reference argument causes crash

2 Answers2