C++: Why is the destructor being called here?

Question

I guess I don't fully understand how destructors work in C++. Here is the sample program I wrote to recreate the issue:

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

using namespace std;

struct Odp
{
    int id;

    Odp(int id)
    {
        this->id = id;
    }

    ~Odp()
    {
        cout << "Destructing Odp " << id << endl;
    }
};

typedef vector<shared_ptr<Odp>> OdpVec;

bool findOdpWithID(int id, shared_ptr<Odp> shpoutOdp, OdpVec& vec)
{
    shpoutOdp.reset();

    for (OdpVec::iterator iter = vec.begin(); iter < vec.end(); iter++)
    {
        Odp& odp = *(iter->get());
        if (odp.id == id)
        {
            shpoutOdp.reset(iter->get());
            return true;
        }
    }

    return false;
}

int main()
{
    OdpVec vec;

    vec.push_back(shared_ptr<Odp>(new Odp(0)));
    vec.push_back(shared_ptr<Odp>(new Odp(1)));
    vec.push_back(shared_ptr<Odp>(new Odp(2)));

    shared_ptr<Odp> shOdp;
    bool found = findOdpWithID(0, shOdp, vec);
    found = findOdpWithID(1, shOdp, vec);
}

Just before main() concludes, the output of this program is:

Destructing Odp 0
Destructing Odp 1

Why does this happen? I'm retaining a reference to each of the Odp instances within the vector. Does it have something to do with passing a shared_ptr by reference?

UPDATE I thought that shared_ptr::reset decremented the ref count, based on MSDN:

The operators all decrement the reference count for the resource currently owned by *this

but perhaps I'm misunderstanding it?

UPDATE 2: Looks like this version of findOdpWithID() doesn't cause the destructor to be called:

bool findOdpWithID(int id, shared_ptr<Odp> shpoutOdp, OdpVec& vec)
{
    for (OdpVec::iterator iter = vec.begin(); iter < vec.end(); iter++)
    {
        Odp& odp = *(iter->get());
        if (odp.id == id)
        {
            shpoutOdp = *iter;
            return true;
        }
    }

    return false;
}

Answer 1

shared_ptr::reset destroys the contents already in the shared_ptr. If you want to affect only that single shared_ptr reference, simply assign to it.

EDIT: In response to comment, you can fix it by changing the body of your for loop to:

if ((*iter)->id == id)
{
    shpoutOdp = *iter;
    return true;
}

EDIT2: That all said, why aren't you using std::find_if here?

#include <iostream>
#include <memory>
#include <vector>
#include <algorithm> //for std::find_if
#include <functional> //for std::bind

struct Odp
{
    int id;

    int GetId()
    {
        return id;
    }

    Odp(int id)
    {
        this->id = id;
    }

    ~Odp()
    {
        std::cout << "Destructing Odp " << id << std::endl;
    }
};

typedef std::vector<shared_ptr<Odp> > OdpVec;

int main()
{
    OdpVec vec;

    vec.push_back(std::shared_ptr<Odp>(new Odp(0)));
    vec.push_back(std::shared_ptr<Odp>(new Odp(1)));
    vec.push_back(std::shared_ptr<Odp>(new Odp(2)));

    OdpVec::iterator foundOdp = std::find_if(vec.begin(), vec.end(), 
        std::bind(std::equal_to<int>(), 0, std::bind(&Odp::GetId,_1)));
    bool found = foundOdp != vec.end();
}

Answer 2

This line right here is probably what is tripping you up.

shpoutOdp.reset(iter->get());

What you're doing here is getting (through get()) the naked pointer from the smart pointer, which won't have any reference tracking information on it, then telling shpoutOdp to reset itself to point at the naked pointer. When shpoutOdp gets destructed, it's not aware that there is another shared_ptr that points to the same thing, and shpoutOdp proceeds to destroy the thing it's pointed to.

You should just do

shpoutOdp = *iter;

which will maintain the reference count properly. As an aside, reset() does decrement the reference counter (and only destroys if the count hits 0).

Answer 3

The nice thing about shared_ptr is that it handles the ref-counting internally. You don't need to manually increment or decrement it ever. (And that is why shared_ptr doesn't allow you to do so either)

When you call reset, it simply sets the current shared_ptr to point to another object (or null). That means that there is now one less reference to the object it pointed to before the reset, so in that sense, the ref counter has been decremented. But it is not a function you should call to decrement the ref counter.

You don't ever need to do that. Just let the shared_ptr go out of scope, and it takes care of decrementing the reference count.

It's an example of RAII in action.

The resource you need to manage (in this case the object pointed to by the shared_ptr) is bound to a stack-allocated object (the shared_ptr itself), so that its lifetime is managed automatically. The shared_ptr's destructor ensures that the pointed-to object is released when appropriate.

Answer 4

So many things that are being used nearly correctly:

bool findOdpWithID(int id, shared_ptr<Odp> shpoutOdp, OdpVec& vec)

Here the parameter shpoutOdp is a a copy of the input parameter. Not such a big deal considering it is a shared pointer but that is probably not what you were intending. You probably wanted to pass by reference otherwise why pass it to the function in the first place.

shpoutOdp.reset();

Resetting a parameter as it is passed in.
Does this mean it could be dirty (then why have it as an input parameter) it make the function return a shared pointer as a result if you want to pass something out.

Odp& odp = *(iter->get());

Don't use get on shared pointers unless you really need to (and you really if ever need too). Extracting the pointer is not necessary to get at what the pointer points at and makes you more likely to make mistakes because you are handling pointers. The equivalent safe(r) line is:

Odp& odp = *(*iter); // The first * gets a reference to the shared pointer.
                     // The second star gets a reference to what the shared 
                     //pointer is pointing at

This is where it all goes wrong:

shpoutOdp.reset(iter->get());

You are creating a new shared pointer from a pointer. Unfortunately the pointer is already being managed by another shared pointer. So now you have two shared pointers that think they own the pointer and are going to delete it when they go out of scope (the first one goes out of scope at the end of the function as it is a copy of the input parameter (rather than a reference)). The correct thing to do is just to do an assignment. Then the shared pointers know they are sharing a pointer:

shpoutOdp = *iter; // * converts the iterator into a shared pointer reference

The next line though not totally wrong does assume that the iterators used are random access (which is true for vector).

for (OdpVec::iterator iter = vec.begin(); iter < vec.end(); iter++)

But this makes the code more brittle as a simple change in the typedef OdpVec will break the code without any warning. So to make this more consistent with normal iterator usage, use != when checking against end() and also prefer the pre increment operator:

for (OdpVec::iterator iter = vec.begin(); iter != vec.end(); ++iter)