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!

• ildjarn: tmp.reset(); only resets tmp, not the object inside of sentences.
• Andreas W. Wylach: @ildjarn: Because it was copied, incremented the reference counter and this is why I only delete tmp ?! I can not use s because the compiler says error: ‘class ArrayStemmedSnippet::StemmedSentence’ has no member named ‘reset’, which I understand. So how do I clearly delete it?
• bitmask: Just a side note: If you make your operator() const, you could pass your EraseSentenceIf object as temporary instead of using the esi variable.

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.

• Andreas W. Wylach: OK, I got it now. Means by simply returning true to remove_if tells erase_if to actually delete the pointer from the vector, right? Because I see that the destructor of the StemmedSentence object is called.
• Andrew Durward: @AndreasW.Wylach Right. Though technically, the deletion could occur during remove_if since it overwrites the items to be removed with those to be kept (hence causing the removed shared_ptr's to be destroyed). erase just removes the garbage from the end of the vector once remove_if is finished.
• Andreas W. Wylach: Ok, thanks for the suggestions! I made the changes and as I see in the allocations count of valgrind, indeed there are less allocations with this change. I basically forget that the tmp assignment makes a copy.

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.

• Domagoj Pandža: +1; OP seems to be having difficulties understanding the founding principles behind smart pointers. I would suggest understanding the problem before taking up the buzzword solution. There are a lot of online resources, just a quick search on SO reveals many useful answers that successfully FAQ-ify the problem: stackoverflow.com/questions/395123/raii-and-smart-pointers-in-c/…
• Andreas W. Wylach: StemmedSentence is an nested class of ArrayStemmedSnippet and needs to point to the current ArrayStemmedSnippet object. I think there is no problem with that.
• Andreas W. Wylach: @Domagoj Pandža: Thank you for your kind suggestion. I am doing that already while I try something out (learning by doing), and I ask here if I am on the right track. Yes, smart pointers are new to me. So forgive me for my beginners mistakes.