Use unique_ptr for ownership and raw pointer otherwise?

Question

I am C++11-ing some code. I have

class X { /* */ };

class A {
    std::vector<X*> va_x;
};

class B {
    std::vector<X*> vb_x;
    std::vector<A> vb_a;
};

The X*s of "va_x" inside my class A point to objects that are also pointed to by the X*s of "vb_x" inside my class B.

Now I would like to use smart pointers. For me, it seems clear that class B has the ownership of the objects pointed by the X* (in particular because my A instances belong to B)

So I should use a unique_ptr for X inside B:

class B {
    std::vector<unique_ptr<X>> vb_x;
    std::vector<A> vb_a;
};

My question is, what should I do for class A? Should I keep raw pointers? By doing so, in my unit tests, I must admit that it leads to awkward things (imo), for instance (don't worry about encapsulation, that's not the point):

unique_ptr<X> x(new X());
A a;
a.va_x.push_back(&(*x)); //awkward, but what else can I do?

A.vb_a.push_back(a); //ok
B.vb_x.push_back(move(x)); //ok

For future questions about C++11, please tag them with C++ aswell. :) — Xeo, Sep 23 '12 at 19:39
@DeadMG Well the previous one is also about unique_ptr but this is not same thing I think — Bérenger, Sep 23 '12 at 19:47
You should probably just not be storing raw pointers in the vector in the first place! Why not put the unique pointers into the vector? — Kerrek SB, Sep 23 '12 at 19:58
@KerrekSB Because these unique_ptr are already in the vector "B::vb_x". I can't put them at the same time in "A::va_x" because they are unique. Unless I missed something about the whole unique thing... — Bérenger, Sep 23 '12 at 20:05
@BérengerBerthoul: In that case, store reference wrappers in `A`, and construct your code so that it is mandatory for the `B` object to exist first. — Kerrek SB, Sep 23 '12 at 20:06
@KerrekSB I don't know ref wrapper, but it seems interesting indeed. Thanks, I will seach for that :) — Bérenger, Sep 23 '12 at 20:18

score 10 · Accepted Answer · edited May 23 '17 at 11:54

10

You can use x.get(), which will return the internal pointer.

Other than that, yes, using raw pointers to handle non-owning references is the way to go, see also this question.

edited May 23 '17 at 11:54

Community

1
1

answered Sep 23 '12 at 19:35

Xeo

129,499
52
291
397

Ok thanks. But this isn't so much better right? So I should use raw pointers anyway ? – Bérenger Sep 23 '12 at 19:48
@BérengerBerthoul: There is *nothing* wrong with using raw pointers as long as they don't own anything. As the name says, they should just point to things and everything is OK. – Xeo Sep 23 '12 at 19:50
Guess it solved it then. I am very cautious about these new features because I did not see any real code with them. And as I heard "replace your raw pointers with smart ones" everywhere, I began to think raw pointers aren't needed anymore (which is not very smart I admit...) Thanks – Bérenger Sep 23 '12 at 20:02
4

@Xeo: I'd like to disagree that there's nothing wrong with raw pointers. Raw pointers are a conceptual nightmare, because they don't have any associated ownership semantics. Either use a unique pointer for ownership, or a reference (wrapper) for ambiently-owned objects. – Kerrek SB Sep 23 '12 at 20:06

Alexis Wilke · Answer 2 · 2018-06-08T01:10:31.860

As Xeo says in his answer, the solution is generally to use x.get().

There is an example of a FILE which uses x.get() whenever it needs to be accessed:

void file_deleter(FILE *f)
{
    fclose(f);
}

[...]

{
    std::unique_ptr<FILE, decltype(&file_deleter)>
                       f(fopen("/tmp/test.tmp", "rw"), &file_deleter);

    // read/write use f.get() as in:
    fread(buf, 1, sizeof(buf), f.get());
    fwrite(buf, 1, sizeof(buf), f.get());

    // fclose() gets called in the deleter, no need to do anything
}

However, in your case, you need to use x.release().

A a;
{
    unique_ptr<X> x(new X());
    a.va_x.push_back(&(*x)); // this is wrong
}
// here a[0] is a dangling pointer

The &(*x) will get the raw pointer and make a copy in the a vector. However, at the time the unique_ptr<> goes out of scope, that pointer gets deleted. So after the }, the pointer in the a vector is not good anymore (although it might work for a little while.)

The correct way of transferring the bare pointer is to use the release() function as in:

    a.va_x.push_back(x.release()); // this works

After that one line, the pointer is only in the a vector. It was released from the unique pointer with the idea that the caller now becomes responsible for managing that resource.

IMPORTANT NOTE: The push_back() may throw (bad_alloc) and if that happens, the resource is lost. To avoid that problem (in case your software catches the bad_alloc and continues to run) you need to first reserve space in the vector as in:

    a.va_x.reserve(a.va_x.size() + 1);  // malloc() happens here
    a.va_x.push_back(x.release());      // no `bad_alloc` possible here

That way, the bad_alloc would happen on that statement while the resource is still attached to the unique_ptr and you do not leak it in case of an exception.

All of that said, you were probably in need of a shared_ptr instead. Those can be copied without trouble. A unique_ptr is more for a resource being allocated once and then forgotten one a function returns or an object is deleted. When (many) copies are involved, a shared_ptr makes more sense.

class X
{
    typedef std::shared_ptr<X> pointer_t;
    [...]
}

class A
{
    std::vector<X::pointer_t> va_x;
}

X::pointer_t x(new X());
A a;
a.va_x.push_back(x); // much cleaner and the pointer is still managed

"However, in your case, you need to use x.release()". No. That completely defeats the point of using a unique pointer since I am now responsible for deleting the pointer. — Bérenger, Jun 09 '18 at 19:04
@Bérenger Well, if you don't then you must make sure that the vector goes out of scope before the `unique_ptr` instance. If you can do that in your specific code, then you're good, indeed. — Alexis Wilke, Jun 09 '18 at 21:09

Use unique_ptr for ownership and raw pointer otherwise?

2 Answers2

Linked