Do I need to reset a shared_ptr before removing it from a vector?

Question

I've written a very simple C++ program using std::shared_ptr.

Here's the code :

/*
** Resource class definition
*/
class Resource
{
    public:
        std::string m_Name;
        Resource(void){}
        Resource(std::string name)
            :   m_Name(name)
        {

        }
        std::string const &GetName(void) const
        {
            return (this->m_Name);
        }
};

namespace Predicate
{
    /*
    ** Predicate - Delete a specific node according to its name
    */
    template <typename T>
    struct DeleteByName
    {
        DeleteByName(std::string const &name);
        bool operator()(T &pData);
        std::string m_Name;
    };

    //Initialization

    template <typename T>
    DeleteByName<T>::DeleteByName(std::string const &name)
        :   m_Name(name)
    {

    }

    //Surcharges

    template <typename T>
    bool DeleteByName<T>::operator()(T &pData)
    {
        if (pData->GetName() == this->m_Name)
        {
            pData.reset();
            return (true);
        }
        return (false);
    }
}

/*
** Remove a specific node according to its name - WORKS
*/
static void RemoveByName__CLASSIC__OK(std::string const &name, std::vector<std::shared_ptr<Resource>> &resourceList)
{
    std::vector<std::shared_ptr<Resource>>::iterator It = resourceList.begin();
    std::vector<std::shared_ptr<Resource>>::iterator It_dest;

    for (; It != resourceList.end(); ++It) {
        if (!(*It)->GetName().compare(name))
        {
            It_dest = It;
        }
    }
    It_dest->reset();
    resourceList.erase(It_dest);
}

/*
** Remove a specific node according to its name - NOT WORK
*/
static void RemoveByName__CLASSIC__NOT_OK(std::string const &name, std::vector<std::shared_ptr<Resource>> &resourceList)
{
    std::vector<std::shared_ptr<Resource>>::iterator It = resourceList.begin();

    for (; It != resourceList.end(); ++It) {
        if (!(*It)->GetName().compare(name))
        {
            It->reset();
            resourceList.erase(It);
        }
    }
}

static std::vector<std::shared_ptr<Resource>>::const_iterator FindByName__PREDICATE__OK(
    std::string const &name, std::vector<std::shared_ptr<Resource>> &resourceList)
{
    return (std::find_if(resourceList.begin(),
            resourceList.end(), Predicate::FindByName<std::shared_ptr<Resource>>(name)));
}

/*
** Remove a specific node according to its name using std::remove_if algorithm with the predicate 'DeleteByName' - WORKS
*/
static void RemoveByName__PREDICATE__OK(std::string const &name, std::vector<std::shared_ptr<Resource>> &resourceList)
{
    if (FindByName__PREDICATE__OK(name, resourceList) != resourceList.end())
        resourceList.erase(std::remove_if(
            resourceList.begin(), resourceList.end(), Predicate::DeleteByName<std::shared_ptr<Resource>>(name)));
}

/*
** Entry point
*/
int main(void)
{
    std::vector<std::shared_ptr<Resource>> resourceList;

    std::shared_ptr<Resource> rsc_A(new Resource("resource_a"));
    std::shared_ptr<Resource> rsc_B(new Resource("resource_b"));
    std::shared_ptr<Resource> rsc_C(new Resource("resource_c"));

    resourceList.push_back(rsc_A);
    resourceList.push_back(rsc_B);
    resourceList.push_back(rsc_C);

    PrintResourceList(resourceList);

    RemoveByName__PREDICATE__OK("resource_as", resourceList);

    PrintResourceList(resourceList);

    getchar();
    return (0);
}

I just want to know if I erase a node from a std::vector containing a shared pointer, if I have to call the method 'reset' to destroy the shared pointer before calling the 'erase' method. I think if I just destroy the node without any call of the function 'reset' the shared pointer should be still destroyed. Is that right?

Plus, I don't understand why the function 'RemoveByName__CLASSIC__NOT_OK' fails. I don't understand why I have to declare an 'It_dest' to store the iterator during the loop (see the method 'RemoveByName__CLASSIC__OK') and finally erase the node at the end of the function. This problem just happened using shared pointer. Does any one have an idea?

Answer 1

RemoveByName__CLASSIC__NOT_OK executes undefined behavior.

When you erase from a std::vector, all iterators and references to elements at or after the point of erasure are invalidated. That means they cannot be dereferenced or compared or anything else but overwritten safely without invoking undefined behavior.

Now, UB is often "does the thing you thought it should do magically", so a failure to crash doesn't help you.

As it happens, if RemoveByName__CLASSIC__NOT_OK did a break immediately after the erase, it would be well defined.

RemoveByName__CLASSIC__OK defers the erase until after you finish the iteration. It has a number of problems, including executing undefined behavior if an element with that name is not there, not handling duplicate names, etc. It erases the last element matching the name if it exists, and otherwise does undefined behavior. You probably want every element, and/or erase the first you find to save time. (If you really want last, iterate backwards and erase the first you find).

.reset() prior to destroying a shared_ptr moves the possible destruction of the object to the .reset() instead of it being inside the guts of std::shared_ptr, which sometimes can be useful (as any and all access to std::vector while you are in the guts is UB). One thing I oftne do is swap or move the shared_ptr out of the container, .erase it from the container, then .reset or just let the local shared_ptr copy go out of scope.

Your RemoveByName__PREDICATE__OK is also broken and can exhibit undefined behavior and basically does the wrong thing if anything except 1 element matching the predicate is found. Change the ); at the end of the erase clause to , resourceList.end());, so that instead of erasing one element it erases everything from the return value of remove_if to the end of the vector.

Answer 2

You don't have to reset the shared_ptr manually, that is done in the destructor. When you erase it, the object gets destroyed and thus decreases the reference count.

Your RemoveByName__CLASSIC__NOT_OK function failes because you are using the iterator after erasing the pointed element. After std::vector::erase, the iterator will be invalid and cannot be used anymore. erase returns the next iterator.

static void RemoveByName__CLASSIC__NOT_OK(std::string const &name, std::vector<std::shared_ptr<Resource>> &resourceList)
{
    for (auto It = resourceList.begin(); 
         It != resourceList.end(); ) {
        if (!(*It)->GetName().compare(name))
        {
            It = resourceList.erase(It);
        }
        else
        {
            ++It;
        }
    }
}

I consider the implementation with remove_if more readable.