[cfe-dev] Use of Smart Pointers in LLVM Projects

[Alp Toker]

On 18/07/2014 03:07, Lang Hames wrote:
> Hi Alp,
>
>     A clear place that smart pointers *shouldn't* be used are 'create'
>     functions.
>
>     There's already a strong convention in the thousands of 'create'
>     functions across the codebase and they have the clear,
>     well-defined behaviour of returning a new pointer.
>
>     We all know what to expect from these functions, much like
>     new/delete in C++ and malloc/free. They can be easily adopted by
>     std::shared_ptr or std::unique_ptr as needed, so there's no
>     benefit in enforcing smart pointer usage at creation.
>
>
> I strongly disagree with this: If knowing that new'ed things must be 
> deleted was enough we would never have to worry about memory leaks. 
> It's a huge benefit to have the compiler catch our mistakes for us - 
> it's one less thing to worry about.

I don't remember the last time we had a leak because of a 'create' 
function. Tricky ownership is going to continue to be tricky either way 
so I'd tend towards keeping the status quo for these functions.

The bad patterns seem come about mostly when smart pointers are used as 
return values. We already avoid returning structures by value, so 
perhaps any policy can be expressed as a continuation of that?



>     Apart from the complexity and visual clutter, it's pretty annoying
>     to have to decide whether to get(), release() or somehow else
>     acquire the object you just created. The result of using smart
>     pointers as returns isn't pretty -- you end up with awkward code
>     like createFoo().get()->func() or other.reset(createFoo().release()).
>

Creating an object for single use and having it destroyed implicitly 
doesn't sound like a great pattern. I almost wonder if it isn't better 
to write the ownership explicitly in such cases:

   unique_ptr<T> t = createFoo();
   t->func();

A createFoo() statement with an unused pointer new return can be 
identified by leak detectors, whereas an unused smart pointer return 
will be created and deleted silently, which isn't that desirable. If you 
really want to delete an object as soon as it's created that seems to be 
worth keeping explicit.

>
> I think if you were ever writing createFoo()->func() before you were 
> leaking memory. And actually you can now safely use the simpler syntax 
> with std::unique_ptr: createFoo()->func() will work fine, there's no 
> need for the .get().
>
> Ditto other.reset(createFoo.release()). I think this would just be: 
> other = createFoo(), (or a = std::move(b);).

The nice thing about a traditional 'create' function is that there's one 
clear way to do that -- by assignment, instead of deciding between two 
or three. It should be equally safe.

>
> There may be occasions where you need to call .get() to pass a pointer 
> as an argument to a function that isn't taking ownership, but that 
> doesn't seem odious to me.
> Cheers,
> Lang.

-- 
http://www.nuanti.com
the browser experts