2
\$\begingroup\$

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!

\$\endgroup\$
3
  • \$\begingroup\$ tmp.reset(); only resets tmp, not the object inside of sentences. \$\endgroup\$
    – ildjarn
    Commented Aug 4, 2012 at 3:01
  • \$\begingroup\$ @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? \$\endgroup\$
    – Andreas W. Wylach
    Commented Aug 4, 2012 at 3:15
  • \$\begingroup\$ Just a side note: If you make your operator() const, you could pass your EraseSentenceIf object as temporary instead of using the esi variable. \$\endgroup\$
    – bitmask
    Commented Aug 4, 2012 at 3:16

2 Answers 2

1
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ 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. \$\endgroup\$
    – Andreas W. Wylach
    Commented Aug 4, 2012 at 3:31
  • \$\begingroup\$ @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. \$\endgroup\$
    – Andrew Durward
    Commented Aug 4, 2012 at 3:49
  • \$\begingroup\$ 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. \$\endgroup\$
    – Andreas W. Wylach
    Commented Aug 4, 2012 at 3:56
2
\$\begingroup\$
  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.

\$\endgroup\$
3
  • 2
    \$\begingroup\$ +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/… \$\endgroup\$
    – Domagoj Pandža
    Commented Aug 4, 2012 at 3:42
  • \$\begingroup\$ StemmedSentence is an nested class of ArrayStemmedSnippet and needs to point to the current ArrayStemmedSnippet object. I think there is no problem with that. \$\endgroup\$
    – Andreas W. Wylach
    Commented Aug 4, 2012 at 3:45
  • \$\begingroup\$ @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. \$\endgroup\$
    – Andreas W. Wylach
    Commented Aug 4, 2012 at 3:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.