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

[David Blaikie]

On Thu, Jul 17, 2014 at 5:21 PM, Alp Toker <alp at nuanti.com> wrote:
>
> 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.

I've explained a way to make pretty simple code even simpler. It
doesn't seem like factory functions returning raw pointers is helping
us.

> The bad patterns seem come about mostly when smart pointers are used as
> return values.

Which bad patterns are you referring to?

> 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();

This would have to be:

unique_ptr<T> t(createFoo());

if createFoo returns a raw pointer.

>   t->func();
>
> A createFoo() statement with an unused pointer new return can be identified
> by leak detectors,

Seems like a bit of a stretch to detect unused variables by using a
leak checker.

> 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.

Actually there isn't and at least one (raw pointer) is casually unsafe:

1) std::unique_ptr<T> foo(createFoo());

2) foo.reset(createFoo());

3) T *foo = createFoo();
  ...
  delete foo;

Just for the initialization - for assignment with raw pointers, that's
where it gets scarier - if you're just using raw assignment, now
you've got two raw pointers pointing to the same object, one of which,
presumably, owns the object.

with unique_ptr, it's more like:

1) std::unique_ptr<T> foo = createFoo();

2) foo = createFoo();

and optionally: auto foo = createFoo();

- David