Refactoring code to use Boost shared pointers

Question

I wrote a project using normal pointers and now I'm fed up with manual memory management.

What are the issues that one could anticipate during refactoring?

Until now, I already spent an hour replacing X* with shared_ptr<X> for types I want to automatically manage memory. Then I changed dynamic_cast to dynamic_pointer_cast. I still see many more errors (comparing with NULL, passing this to a function).

I know the question is a bit vague and subjective, but I think I can benefit from experience of someone who has already done this.

Are there some pitfalls?

Answer 1

I'm listing the steps/issues involved. They worked for me, but I can't vouch that they are 100% correct

0) check if there could be cyclic shared pointers. If so, can this lead to memory leak? I my case, luckily, cycles need not be broken because if I had a cycle, the objects in the cycle are useful and should not be destroyed. use weak pointers to break cycles

1) you need to replace "most" X* with shared_ptr<X> . A shared_ptr is (only?) created immediately after every dynamic allocation of X . At all other times, it is copy constructed , or constructed with an empty pointer(to signal NULL) . To be safe (but a bit inefficient), pass these shared_ptrs only by reference . Anyways, it's likely that you never passed your pointers by reference to begin with => no additional change is required

2) you might have used dynamic_cast<X*>(y) at some places. replace that with dynamic_pointer_cast<X>(y)

3) wherever you passed NULL(eg. to signal that a computation failed), pass an empty shared pointer.

4) remove all delete statements for the concerned types

5) make your base class B inherit from enable_shared_from_this<B>. Then wherever you passed this , pass, shared_from_this() . You might have to do static casting if the function expected a derived type . keep in mind that when you call shared_from_this(), some shared_ptr must already be owning this . In particular, don't call shared_from_this() in constructor of the class

I'm sure one could semi-automate this process to get a semantically equivalent but not necessarily very-efficient code. The programmer probably only needs to reason about cyclic reference(if any).

I used regexes a lot in many of these steps. It took about 3-4 hours. The code compiles and has executed correctly so far.

Answer 2

There is a tool that tries to automatically convert to smart pointers. I've never tried it. Here is a quote from the abstract of the following paper: http://www.cs.rutgers.edu/~santosh.nagarakatte/papers/ironclad-oopsla2013.pdf

To enforce safety properties that are difficult to check statically, Ironclad C++ applies dynamic checks via templated “smart pointer” classes. Using a semi-automatic refactoring tool, we have ported nearly 50K lines of code to Ironclad C++

Answer 3

If you wish to stick with boost, you should consider if you want a boost::shared_ptr or a boost::scoped_ptr. A shared_ptr is a resource to be shared between classes, whereas a scoped_ptr sounds more like what you may want (at least in some places). A scoped_ptr will automatically delete the memory when it goes out of scope.

Be wary when passing a shared_ptr to a function. The general rule with shared_ptr is to pass by value so a copy is created. If you pass it by reference then the pointer's reference count will not be incremented. In this case, you might end up deleting a piece of memory that you wanted kept alive.

There is a case, however, when you might want to pass a shared_ptr by reference. That is, if you want the memory to be allocated inside a different function. In this case, just make sure that the caller still holds the pointer for the lifetime of the function it is calling.

void allocPtr( boost::shared_ptr< int >& ptrByRef )
{
    ptrByRef.reset( new int );
    *ptrByRef = 3;
}

int main()
{
    boost::shared_ptr< int >& myPointer;
    // I want a function to alloc the memory for this pointer.

    allocPtr( myPointer ); // I must be careful that I still hold the pointer
                           // when the function terminates

    std::cout << *ptrByRef << std::endl;
}

Answer 4

Are there some pitfalls?

Yes, by murphy's law if you blindly replace every pointer with shared_ptr, it'll turn out that isn't what you wanted, and you'll spend next 6 months hunting bugs you introduced.

What are the issues that one could anticipate during refactoring?

Inefficient memory management, unused resources being stored longer than necessary, memory leaks (circular references), invalid reference counting (same pointer assigned to multiple different shared_pointers).

Do NOT blindly replace everything with shared_ptr. Carefully investigate program structure and make sure that shread_ptr is NEEDED and it represents EXACTLY what you want.

Also, make sure you use version control that supports easy branching (git or mercurial), so when you break something you can revert to previous state or run something similar to "git bisect" to locate problem.

obviously you need to replace X* with shared_ptr

Wrong. It depends on context. If you have a pointer that points into the middle of some array (say, pixel data manipulation), then you won't be able to replace it with shared_ptr (and you won't need to). You need to use shared_ptr only when you need to ensure automatic deallocation of object. Automatic deallocation of object isn't always what you want.

Answer 5

Although it's easy to just use boost::shared_pointer everywhere, you should use the correct smart pointer as per ownership semantics.

In most cases, you will want to use std::unique_ptr by default, unless the ownership is shared among multiple object instances.

If you run into cyclical ownership problems, you can break up the cycles with boost::weak_ptr.

Also keep in mind that while passing shared_ptr's around, you should always pass them by const reference for performance reasons (avoids an atomic increment) unless you really want to confer ownership to a different entity.