Best Practice for Scoped Reference Idiom?

Question

I just got burned by a bug that is partially due to my lack of understanding, and partially due to what I think is suboptimal design in our codebase. I'm curious as to how my 5-minute solution can be improved.

We're using ref-counted objects, where we have AddRef() and Release() on objects of these classes. One particular object is derived from the ref-count object, but a common function to get an instance of these objects (GetExisting) hides an AddRef() within itself without advertising that it is doing so. This necessitates doing a Release at the end of the functional block to free the hidden ref, but a developer who didn't inspect the implementation of GetExisting() wouldn't know that, and someone who forgets to add a Release at the end of the function (say, during a mad dash of bug-fixing crunch time) leaks objects. This, of course, was my burn.

void SomeFunction(ProgramStateInfo *P)
{
   ThreadClass *thread = ThreadClass::GetExisting( P );
   // some code goes here
   bool result = UseThreadSomehow(thread);
   // some code goes here
   thread->Release();  // Need to do this because GetExisting() calls AddRef()
}

So I wrote up a little class to avoid the need for the Release() at the end of these functions.

class ThreadContainer
{
private:
    ThreadClass *m_T;
public:
    ThreadContainer(Thread *T){ m_T = T; }
    ~ThreadContainer() { if(m_T) m_T->Release(); }
    ThreadClass * Thread() const { return m_T; }
};

So that now I can just do this:

void SomeFunction(ProgramStateInfo *P)
{
   ThreadContainer ThreadC(ThreadClass::GetExisting( P ));
   // some code goes here
   bool result = UseThreadSomehow(ThreadC.Thread());
   // some code goes here
   // Automagic Release() in ThreadC Destructor!!!
}

What I don't like is that to access the thread pointer, I have to call a member function of ThreadContainer, Thread(). Is there some clever way that I can clean that up so that it's syntactically prettier, or would anything like that obscure the meaning of the container and introduce new problems for developers unfamiliar with the code?

Thanks.

Answer 1

Take a look at ScopeGuard. It allows syntax like this (shamelessly stolen from that link):

{
    FILE* topSecret = fopen("cia.txt");
    ON_BLOCK_EXIT(std::fclose, topSecret);
    ... use topSecret ...
} // topSecret automagically closed

Or you could try Boost::ScopeExit:

void World::addPerson(Person const& aPerson) {
    bool commit = false;
    m_persons.push_back(aPerson);  // (1) direct action
    BOOST_SCOPE_EXIT( (&commit)(&m_persons) )
    {
        if(!commit)
            m_persons.pop_back(); // (2) rollback action
    } BOOST_SCOPE_EXIT_END

    // ...                        // (3) other operations

    commit = true;                // (4) turn all rollback actions into no-op
}

Answer 2

Yes, you can implement operator ->() for the class, which will recursively call operator ->() on whatever you return:

class ThreadContainer
{
private:
    ThreadClass *m_T;
public:
    ThreadContainer(Thread *T){ m_T = T; }
    ~ThreadContainer() { if(m_T) m_T->Release(); }
    ThreadClass * operator -> () const { return m_T; }
};

It's effectively using smart pointer semantics for your wrapper class:

Thread *t =  new Thread();
...
ThreadContainer tc(t);
...
tc->SomeThreadFunction(); // invokes tc->t->SomeThreadFunction() behind the scenes...

You could also write a conversion function to enable your UseThreadSomehow(ThreadContainer tc) type calls in a similar way.

If Boost is an option, I think you can set up a shared_ptr to act as a smart reference as well.

Answer 3

use boost::shared_ptr it is possible to define your own destructor function, such us in next example: http://www.boost.org/doc/libs/1_38_0/libs/smart_ptr/sp_techniques.html#com

Answer 4

You can add an automatic type-cast operator to return your raw pointer. This approach is used by Microsoft's CString class to give easy access to the underlying character buffer, and I've always found it handy. There might be some unpleasant surprises to be discovered with this method, as in any time you have an implicit conversion, but I haven't run across any.

class ThreadContainer
{
private:
    ThreadClass *m_T;
public:
    ThreadContainer(Thread *T){ m_T = T; }
    ~ThreadContainer() { if(m_T) m_T->Release(); }
    operator ThreadClass *() const { return m_T; }
};

void SomeFunction(ProgramStateInfo *P)
{
   ThreadContainer ThreadC(ThreadClass::GetExisting( P ));
   // some code goes here
   bool result = UseThreadSomehow(ThreadC);
   // some code goes here
   // Automagic Release() in ThreadC Destructor!!!
}

Answer 5

I would recommend following bb advice and using boost::shared_ptr<>. If boost is not an option, you can take a look at std::auto_ptr<>, which is simple and probably addresses most of your needs. Take into consideration that the std::auto_ptr has special move semantics that you probably don't want to mimic.

The approach is providing both the * and -> operators together with a getter (for the raw pointer) and a release operation in case you want to release control of the inner object.