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

[Lang Hames]

Hi Alp,

> I don't remember the last time we had a leak because of a 'create' function.

I'm not sure I follow you here. Are you suggesting that we've never leaked memory returned by a create function? I'll wager we have. I'm also certain that we've double deleted such memory, which is something unique_ptr should also prevent.

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

Is there an official injunction against returning structs by value? Perhaps we should reconsider that in light of move semantics? In any case I think the only reason to avoid returning structs by value is the overhead of the copies, which doesn't apply to unique_ptr. Any policy regarding smart pointers use in return types should be separate from the struct usage policy.

> 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 only mentioned it since you offered it as an example of something that would be uncomfortable to write, which it turns out not to be. As you say - this is very unlikely to come up at all. The idea of using memory leaks to diagnose dead code sounds less than ideal.

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

The only difference between regular pointers and smart pointers is precisely the difference you want: Smart pointers keep the ownership explicit. Consider:

Foo *A = createFoo();
Foo *B = A; // Who owns the object now?

vs

std::unique_ptr<Foo> A = createFoo();
std::unique_ptr<Foo> B = std::move(A); // Clear transfer of ownership.

David mentioned other smart pointer types (e.g. shared_ptr) in his email, and I should be clear: I'm advocating for std::unique_ptr usage where appropriate, as that's what I have experience with. I'm less familiar with other smart pointers, and happy to hear from the experts on those.

Cheers,
Lang.