Is this predicate valid to delete single shared_ptr's?

Question

I wrote a predicate used in a remove_if call that deletes shared_ptr's of type StemmedSentence from an vector of sentences.

The predicate:

class EraseSentenceIf {
    ArrayStemmedSnippet * m_ass;

public:
    EraseSentenceIf(ArrayStemmedSnippet *ass)
    : m_ass(ass) {
    }

    bool operator()(const std::shared_ptr<
        ArrayStemmedSnippet::StemmedSentence>& s) {
        std::shared_ptr<ArrayStemmedSnippet::StemmedSentence> tmp = s;

        // --- set StemmedSentnce object in ArrayStemmedSnippet class
        s->setParent(m_ass);

        // --- if true delete this sentence)
        if (s->trimStopWords()) {
            tmp.reset();

            return true;
        }

        return false;
    }
};

The remove_if call:

EraseSentenceIf esi(this);
sentences.erase(
    std::remove_if(
        sentences.begin(), sentences.end(), esi),
    sentences.end()
);

Declaration:

std::vector<shared_ptr<StemmedSentence> > sentences;

The construction of the sentences objects looks like this:

sentences.push_back(shared_ptr<StemmedSentence>(
    new StemmedSentence(index, i - 1 )));

The code seems to run fine, valgrind / gdb does not moan. I just want to get sure that I handle the deletion (or release) of the shared_ptr in a correct way. Can somebody please confirm this? Maybe I can improve something or I overlooked an important point. Thanks for your comments!

Answer 1

Within the predicate you make a copy of the shared_ptr hence incrementing the reference count:

    std::shared_ptr<ArrayStemmedSnippet::StemmedSentence> tmp = s;

A few lines later you explicitly reset this copy (note that this does not release any memory unless it's the last living shared_ptr referring to the pointee):

    // --- if true delete this sentence)
    if (s->trimStopWords()) {
        // NOT NECESSARY -- reference count will be decremented when tmp falls out of scope
        tmp.reset();

        return true;
    }

The actual deletion occurs when the shared_ptr residing inside the vector is destroyed (assuming it's the last remaining copy):

sentences.erase(
    std::remove_if(
        sentences.begin(), sentences.end(), esi),
    sentences.end()
);

So, everything will work fine as it is but the tmp variable in the predicate is unnecessary.

Answer 2

1. The copy of the pointer is completely unnecessary: as long as the code is not sharing shared_ptrs among threads (which is really, really silly), taking a shared_ptr argument by reference ensures it lives at least until the function finishes executing.

2. Even if the copy was necessary, passing by reference to immediately make a copy is a pessimisation as it forbids moves. Don't pass shared_ptr by reference if you're going to copy it.

3. The pointer in the vector will be destroyed by erase, there's no need to manually reset anything.

4. A predicate that mutates its argument is bound to raise eyebrows. Depending on the semantics of setParent it may or may not be problematic, but it is something I'd avoid if I could.