Proper Destructors / RAII / Something

Question

Here's a minimal example of a problem that I'm having, and I can't work out how I should solve it:

#include <vector>
#include <memory>

class Thing {
};

class App {
public:
    std::vector<std::unique_ptr<Thing>> thingVec;
    void add_thing(Thing*);
};

void App::add_thing(Thing* thing) {
    thingVec.push_back(std::unique_ptr<Thing>(thing));
}

int main() {
    App app;

    Thing thing;

    app.add_thing(&thing);
}

This compiles and runs with no issues, however, upon reaching the end of main, segfaults and spits out:

Error in `/path/testapp': free(): invalid pointer: 0x00007fff97118070 ***

Any possible help? The reason I want to store (unique) pointers is that Thing will usually be derived.

EDIT: One working solution:

#include <vector>
#include <memory>

class Thing {
};

class App {
public:
    std::vector<std::unique_ptr<Thing>> thingVec;
    void add_thing(Thing*);
};

void App::add_thing(Thing* thing) {
    thingVec.push_back(std::unique_ptr<Thing>(thing));
}

int main() {
    App app;

    Thing* thing = new Thing;
    app.add_thing(thing);
}

But from what I understand, I should be able to avoid using new entirely, and use make_unique? I can't seem to find where make_unique is actually defined though.

EDIT 2:

Is this more appropriate? Is there a less messy looking way to do this? Otherwise, it works well.

#include <vector>
#include <memory>
#include <iostream>

class Thing {
public:
    int foo = 42;
};

class App {
public:
    std::vector<std::unique_ptr<Thing>> thingVec;
    void add_thing(std::unique_ptr<Thing>);
};

void App::add_thing(std::unique_ptr<Thing> thing) {
    thingVec.push_back(std::move(thing));
}

int main() {
    App app;
    app.add_thing(std::unique_ptr<Thing>(new Thing()));

    std::cout << app.thingVec.back()->foo << std::endl;
}

Because I may end up with lines like

app.thingVex.back()->barVec.back()->blahMap.emplace("flop", std::unique_ptr<Tree>(new Tree));

The std::unique_ptr destroys the object when it goes out of scope. However the pointed to object is already destroyed because it is declared on the stack. — Matt Coubrough, May 27 '14 at 07:05
They "forgot" to specify `make_unique` in C++11. They fixed it in C++14. Until then just write your own `make_unique`, it is [fairly easy](http://stackoverflow.com/questions/17902405/how-to-implement-make-unique-function-in-c11). — nwp, May 27 '14 at 07:24
See also: [Guideline: Express a "sink" function using a by-value `unique_ptr` parameter.](http://herbsutter.com/2013/06/05/gotw-91-solution-smart-pointer-parameters/) — CB Bailey, May 27 '14 at 07:39

score 6 · Answer 1 · answered May 27 '14 at 07:04

6

std::unique_ptr is attempting to delete a stack-allocated Thing instance.

Your error is essentially in the lines:

 Thing thing;
 app.add_thing(&thing);

answered May 27 '14 at 07:04

Bathsheba

231,907
34
361
483

score 1 · Answer 2 · answered May 27 '14 at 07:05

1

You should pass local object to unique_ptr.

Replace

Thing thing;
app.add_thing(&thing);

with

app.add_thing(new Thing);

If you want to also edit the object

Thing *thing = new Thing;
// thing->some_val = val;
app.add_thing(thing);

Make sure not to add the same object twice in the app, as std::unique_ptr take the ownership of pointer pointer would be tried to free more than 1 time.

answered May 27 '14 at 07:05

Mohit Jain

30,259
8
73
100

1

@Bathsheba: but even with this solution the interface to `add_thing` is dangerous because it doesn't prevent this sort of client "error" or document in any way that this usage is problematic. – CB Bailey May 27 '14 at 07:09
Ahhh. Ok. The reason I was trying to initialise them before is because I was trying to run some other methods on them before adding them. I suppose I would just have to add them first, and the run the methods on the last element of the vector? – Jagoly May 27 '14 at 07:10
Yes I should have written "a possible solution", but I'll leave it now as it will invalidate more helpful comments. – Bathsheba May 27 '14 at 07:12
@CharlesBailey You are right, the interface is dangerous and let do the nasty things. – Mohit Jain May 27 '14 at 07:12

CB Bailey · Accepted Answer · 2014-05-27T07:25:09.730

1

The interface to add_thing is wrong because it takes a "non-owning" raw pointer to a Thing and then assumes that it can take full ownership of the object passed in by constructing a unique_ptr from it.

If you change add_thing to take a unique_ptr<Thing> by value, then the caller will be prevented from implicitly converting a raw pointer and will no that they need to construct a new unique_ptr to a heap allocated thing into the add_thing function.

e.g.

void App::add_thing(std::unique_ptr<Thing> thing) {
    thingVec.push_back(std::move(thing));
}

int main() {
    App app;    
    app.add_thing(std::make_unique<Thing>());
}

(Note that std::make_unique is a future feature; std::unique_ptr<Thing>(new Thing) works now.)

edited May 27 '14 at 07:25

answered May 27 '14 at 07:05

CB Bailey

755,051
104
632
656

An alternative to the `unique_ptr` is passing a `Thing &&` which can then be `move`ed into the `unique_ptr`. – nwp May 27 '14 at 07:10
Ok, I'll look into that. I had that originally, but it was rather clunky having to create the unique_ptrs manually beforehand – Jagoly May 27 '14 at 07:11
@nwp what will two & do? – Jagoly May 27 '14 at 07:12
@nwp: Which works but does also require `Thing` to be movable. – CB Bailey May 27 '14 at 07:16
@Jagoly A `Test &&` is a temporary value ([rvalue](http://thbecker.net/articles/rvalue_references/section_03.html)). When using a value from the stack you will get a compiler error saying it cannot convert an lvalue to an rvalue, thus preventing this kind of error. – nwp May 27 '14 at 07:18
@CharlesBailey True. Honestly I have never had to deal with any `Thing` that is not movable. It probably makes sense to offer both functions. – nwp May 27 '14 at 07:21
@nwp: It might make sense to provide both but as `App` isn't a template function you'd only do this if `Thing` actually was movable. (If `Thing` was an olde C++03 class with a user-defined copy constructor then moving it might not be as efficient as "move" suggests.) – CB Bailey May 27 '14 at 07:23
You can write your own `make_unique`. It is [fairly easy](http://stackoverflow.com/questions/17902405/how-to-implement-make-unique-function-in-c11) and solved enough problems to make it worth it. – nwp May 27 '14 at 07:27

score 0 · Answer 4 · 2014-05-27T07:27:09.447

You are transferring the ownership not properly. You might utilize a shared pointer with a custom deleter to the prevent deletion of the referenced variable:

#include <vector>
#include <memory>

class Thing {
};

class App {
public:
    std::vector<std::shared_ptr<Thing>> thingVec;
    void add_thing(std::shared_ptr<Thing>&& thing) {
        thingVec.push_back(std::move(thing));
    }
};


template<typename T>
inline std::shared_ptr<T> make_no_delete(T& value)
{
    return std::shared_ptr<T>(&value, [](void*){});
}


int main() {
    App app;

    Thing thing;

    // Add without transferring ownership:
    app.add_thing(make_no_delete(thing));
    // Add and transfer ownership:
    app.add_thing(std::make_shared<Thing>());
}

Proper Destructors / RAII / Something

4 Answers4