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

On Thu, Jul 17, 2014 at 4:45 PM, Alp Toker <alp at nuanti.com>
wrote:
>
> On 18/07/2014 02:21, David Blaikie wrote:
>>
>> Are people OK with/prefer the use of owning smart pointers in APIs?
>
>
> I think smart pointers are great to use for storage, or as struct members
> where there's actually a clear need for ownership. Just by virtue of
getting
> rid of destructors containing single delete expressions this is already a
> win.
Great!
> In many cases there shouldn't be a need for smart pointers at all,
because
> the object is owned by a central facility's smart pointer so it's
safe to
> just pass around the pointer in ephemeral contexts like stack-based
> instances. In such context raw pointers and references aren't scary --
> they're just fine.
There's no contention there - if the pointer is non-owning, there's no
smart pointer to use. Indeed using a smart pointer for a non-owning
pointer would actually break the code by causing double deletes, etc.

Well, shared_ptr notwithstanding - but, yes, choosing between
unique_ptr + non-owning pointers and shared_ptr can sometimes be
non-obvious, but I'm hopeful we generally agree that if there is a
dominating owner they can be given exclusive ownership through a
unique_ptr and everyone else can use raw pointers.
> (All the above goes equally for uniquely owned pointers, shared pointers
and
> intrusive refcounted pointers.)
>
>
>> Are there places where you've found them to be too noisy/burdensome
>> and would rather use raw pointers or some other abstraction?
>
>
> 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,
They'd actually only ever need to return std::unique_ptr.
std::shared_ptrs can be constructed from xvalue std::unique_ptrs
without any overhead:

std::unique_ptr<T> create();
std::shared_ptr<T> x = create();

(make_shared has a small allocation (and destruction) benefit that
might sometimes be useful to exploit, of course, but you don't benefit
from that with raw new either, so unique_ptr is no /worse/ off than a
raw implicitly-owning pointer in that regard)
> so there's no benefit in enforcing smart pointer usage at creation.
I believe there is. Like const member functions it helps ensure that
ownership is transferred clearly. It's not hard to write bugs like:

T *t = create();
...
if (error)
  return; // leak

set_thing(t); // transfer ownership here

By making create return a std::unique_ptr, it discourages these kinds
of misuses.
> 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.
That generally shouldn't be complicated unless the consumer has weird
ownership semantics, in which case that's worth highlighting.
>The result of using smart pointers as returns isn't pretty --
> you end up with awkward code like createFoo().get()->func()
This isn't an expression you should need to write, it'd simply be
"createFoo()->func()".
> or> other.reset(createFoo().release()).
This is also an expression you shouldn't need to write, it'd simply be
"other = createFoo()". Indeed the alternative here (if createFoo
returns a raw pointer) is "other.reset(createFoo())" which is, imho,
less obvious than simple assignment.
> Transfer of new pointers from 'create' functions and usage is
already
> trivially clear so we shouldn't change these.
The need to use reset rather than assignment and the ease with which
ownership can become uncertain once you leave the create function
don't seem to me to be ideal outcomes.

Ultimately, by moving to a world in which objects are most often
allocated with make_unique (or make_shared) means things like reset,
release, raw new and delete can be given extra scrutiny and memory
management bugs can be better avoided.

For myself, it also just helps me follow/understand code when I can
see ownership explicit in the code rather than having to trace back
through the pointer assignments and function calls.

- David
>> Would you
>> prefer pre-commit review of such changes to adequately consider
>> alternatives (such as?)?
>
> The clear-cut two use-cases above are reasonable for post-commit review,
> certainly in the modules I'm helping to maintain.
>
> The creation functions shouldn't be changed because of the reasons
given
> above, and I think it makes sense to put that in policy rather than making
> the decision on a per-module basis.
>
> Thanks for evaluating this David!
>
> --
> http://www.nuanti.com
> the browser experts
>

On Thu, Jul 17, 2014 at 4:58 PM, David Blaikie <dblaikie at gmail.com>
wrote:
> On Thu, Jul 17, 2014 at 4:45 PM, Alp Toker <alp at nuanti.com> wrote:
> > In many cases there shouldn't be a need for smart pointers at all,
> because
> > the object is owned by a central facility's smart pointer so
it's safe to
> > just pass around the pointer in ephemeral contexts like stack-based
> > instances. In such context raw pointers and references aren't
scary --
> > they're just fine.
>
> There's no contention there - if the pointer is non-owning, there's
no
> smart pointer to use. Indeed using a smart pointer for a non-owning
> pointer would actually break the code by causing double deletes, etc.
>
> Well, shared_ptr notwithstanding - but, yes, choosing between
> unique_ptr + non-owning pointers and shared_ptr can sometimes be
> non-obvious, but I'm hopeful we generally agree that if there is a
> dominating owner they can be given exclusive ownership through a
> unique_ptr and everyone else can use raw pointers.

What about weak_ptr?
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140717/bd77f2a8/attachment.html>

On Thu, Jul 17, 2014 at 8:20 PM, Zachary Turner <zturner at google.com>
wrote:
>
>
>
> On Thu, Jul 17, 2014 at 4:58 PM, David Blaikie <dblaikie at
gmail.com> wrote:
>>
>> On Thu, Jul 17, 2014 at 4:45 PM, Alp Toker <alp at nuanti.com>
wrote:
>> > In many cases there shouldn't be a need for smart pointers at
all,
>> > because
>> > the object is owned by a central facility's smart pointer so
it's safe
>> > to
>> > just pass around the pointer in ephemeral contexts like
stack-based
>> > instances. In such context raw pointers and references aren't
scary --
>> > they're just fine.
>>
>> There's no contention there - if the pointer is non-owning,
there's no
>> smart pointer to use. Indeed using a smart pointer for a non-owning
>> pointer would actually break the code by causing double deletes, etc.
>>
>> Well, shared_ptr notwithstanding - but, yes, choosing between
>> unique_ptr + non-owning pointers and shared_ptr can sometimes be
>> non-obvious, but I'm hopeful we generally agree that if there is a
>> dominating owner they can be given exclusive ownership through a
>> unique_ptr and everyone else can use raw pointers.
>
>
> What about weak_ptr?
If you need the auto-nulling behavior (usually because you've got a
non-owning, but longer-lifetime than the owner), sure. & maybe if
needed we can do something like AssertingVH (essentially a weak_ptr
that asserts on null access in debug builds and it's just a raw
pointer in release builds - a debugging aid for the cases where you
think you can use a raw pointer (shorter lifetime than the owning
pointer) but are investigating possible lifetime bugs) around
weak_ptr.

- David

On Jul 17, 2014 8:20 PM, "Zachary Turner" <zturner at
google.com> wrote:
>
>
>
>
> On Thu, Jul 17, 2014 at 4:58 PM, David Blaikie <dblaikie at
gmail.com> wrote:
>>
>> On Thu, Jul 17, 2014 at 4:45 PM, Alp Toker <alp at nuanti.com>
wrote:
>> > In many cases there shouldn't be a need for smart pointers at
all,
because
>> > the object is owned by a central facility's smart pointer so
it's safe
to
>> > just pass around the pointer in ephemeral contexts like
stack-based
>> > instances. In such context raw pointers and references aren't
scary --
>> > they're just fine.
>>
>> There's no contention there - if the pointer is non-owning,
there's no
>> smart pointer to use. Indeed using a smart pointer for a non-owning
>> pointer would actually break the code by causing double deletes, etc.
>>
>> Well, shared_ptr notwithstanding - but, yes, choosing between
>> unique_ptr + non-owning pointers and shared_ptr can sometimes be
>> non-obvious, but I'm hopeful we generally agree that if there is a
>> dominating owner they can be given exclusive ownership through a
>> unique_ptr and everyone else can use raw pointers.
>
>
> What about weak_ptr?
I'd generally reserve them for use when the auto-null behavior is
necessary, for example in lazy caches. Though it seems the situations that
call for them are relatively rare - its a good tool when you need it.

I suppose in some cases it might be nice to have something like AssertingVH
that's a raw pointer in release builds and an asserting weak_ptr-like thing
in debug builds.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140810/94a069aa/attachment.html>