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

There seems to be some uncertainty about the use of smart pointers
(previously OwningPtr, now std::unique_ptr and std::shared_ptr
predominantly) in the LLVM project as a whole, so here's a thread to
discuss/clarify/etc the project preferences/direction with regard to
smart pointer usage.

For some context, see discussions in LLVM r212403 and Clang r213307.

The basic question here seems to be whether smart pointer ownership is
a direction we want to take the LLVM project in general.

I've seen others contribute and have myself contributed many patches
moving towards smart pointer ownership (both in the pre-C++11 days of
OwningPtr, and much moreso in the post-C++11 world with
std::unique_ptr and std::shared_ptr being usable inside containers, as
return values, etc, allowing many more opportunities).

std::unique_ptr's been used in LLD as far back as r153620.
std::unique_ptr appeared in LLVM shortly after the C++11 switch with
Ahmed's work to migrate the project from OwningPtr to std::unique_ptr
(starting with r202609 and ending with r211259). Originally OwningPtr
was added in r45261.
Something in the order of 60 changes across clang and LLVM mention
unique_ptr in their subject and migrate various APIs to use unique_ptr
for ownership. Many of which remove uses of explicit delete or helpers
like DeleteContainerPointers (and removing explicit dtors in many of
those cases).

Are people OK with/prefer the use of owning smart pointers in APIs?
Are there places where you've found them to be too noisy/burdensome
and would rather use raw pointers or some other abstraction? Would you
prefer pre-commit review of such changes to adequately consider
alternatives (such as?)?

Thanks,
- David

Where unique_ptr fits the memory ownership model I'm all for using it. The
syntactic overhead is a small price to pay for self-documentation and
compile-time guarantees.

What are the objections?

Cheers,
Lang.



On Thu, Jul 17, 2014 at 4:21 PM, David Blaikie <dblaikie at gmail.com>
wrote:
> There seems to be some uncertainty about the use of smart pointers
> (previously OwningPtr, now std::unique_ptr and std::shared_ptr
> predominantly) in the LLVM project as a whole, so here's a thread to
> discuss/clarify/etc the project preferences/direction with regard to
> smart pointer usage.
>
> For some context, see discussions in LLVM r212403 and Clang r213307.
>
> The basic question here seems to be whether smart pointer ownership is
> a direction we want to take the LLVM project in general.
>
> I've seen others contribute and have myself contributed many patches
> moving towards smart pointer ownership (both in the pre-C++11 days of
> OwningPtr, and much moreso in the post-C++11 world with
> std::unique_ptr and std::shared_ptr being usable inside containers, as
> return values, etc, allowing many more opportunities).
>
> std::unique_ptr's been used in LLD as far back as r153620.
> std::unique_ptr appeared in LLVM shortly after the C++11 switch with
> Ahmed's work to migrate the project from OwningPtr to std::unique_ptr
> (starting with r202609 and ending with r211259). Originally OwningPtr
> was added in r45261.
> Something in the order of 60 changes across clang and LLVM mention
> unique_ptr in their subject and migrate various APIs to use unique_ptr
> for ownership. Many of which remove uses of explicit delete or helpers
> like DeleteContainerPointers (and removing explicit dtors in many of
> those cases).
>
> Are people OK with/prefer the use of owning smart pointers in APIs?
> Are there places where you've found them to be too noisy/burdensome
> and would rather use raw pointers or some other abstraction? Would you
> prefer pre-commit review of such changes to adequately consider
> alternatives (such as?)?
>
> Thanks,
> - David
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140717/40434c8f/attachment.html>

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.

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.

(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, so there's no benefit in enforcing smart 
pointer usage at creation.

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

Transfer of new pointers from 'create' functions and usage is already 
trivially clear so we shouldn't change these.

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

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.

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

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140717/7bc2545a/attachment.html>

On Thu, Jul 17, 2014 at 4:43 PM, Lang Hames <lhames at gmail.com> wrote:
> Where unique_ptr fits the memory ownership model I'm all for using it.
The
> syntactic overhead is a small price to pay for self-documentation and
> compile-time guarantees.
>
+1. I liked the patches that added unique_ptrs I've seen so far. WebKit
also heavily uses pointer classes (OwnPtr etc) heavily, and I think it
works well there too.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140717/9af9a591/attachment.html>

+1. I think there is a lot of value in using standard library's smart
pointers over custom smart pointers or raw pointers where the callers are
expected to understand the object’s lifetime. They communicate the intended
semantics to users (leading to less errors), and because they are broadly used
and well defined they will be more amenable to tooling both debugging and
optimization advances in the future.

Louis
> On Jul 17, 2014, at 4:43 PM, Lang Hames <lhames at gmail.com> wrote:
> 
> Where unique_ptr fits the memory ownership model I'm all for using it.
The syntactic overhead is a small price to pay for self-documentation and
compile-time guarantees.
> 
> What are the objections?
> 
> Cheers,
> Lang.
> 
> 
> 
> On Thu, Jul 17, 2014 at 4:21 PM, David Blaikie <dblaikie at
gmail.com> wrote:
> There seems to be some uncertainty about the use of smart pointers
> (previously OwningPtr, now std::unique_ptr and std::shared_ptr
> predominantly) in the LLVM project as a whole, so here's a thread to
> discuss/clarify/etc the project preferences/direction with regard to
> smart pointer usage.
> 
> For some context, see discussions in LLVM r212403 and Clang r213307.
> 
> The basic question here seems to be whether smart pointer ownership is
> a direction we want to take the LLVM project in general.
> 
> I've seen others contribute and have myself contributed many patches
> moving towards smart pointer ownership (both in the pre-C++11 days of
> OwningPtr, and much moreso in the post-C++11 world with
> std::unique_ptr and std::shared_ptr being usable inside containers, as
> return values, etc, allowing many more opportunities).
> 
> std::unique_ptr's been used in LLD as far back as r153620.
> std::unique_ptr appeared in LLVM shortly after the C++11 switch with
> Ahmed's work to migrate the project from OwningPtr to std::unique_ptr
> (starting with r202609 and ending with r211259). Originally OwningPtr
> was added in r45261.
> Something in the order of 60 changes across clang and LLVM mention
> unique_ptr in their subject and migrate various APIs to use unique_ptr
> for ownership. Many of which remove uses of explicit delete or helpers
> like DeleteContainerPointers (and removing explicit dtors in many of
> those cases).
> 
> Are people OK with/prefer the use of owning smart pointers in APIs?
> Are there places where you've found them to be too noisy/burdensome
> and would rather use raw pointers or some other abstraction? Would you
> prefer pre-commit review of such changes to adequately consider
> alternatives (such as?)?
> 
> Thanks,
> - David
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
> 
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140718/5215b2a0/attachment.html>

I don't have much to add here besides +1. I think using std::unique_ptr even
for create* functions/methods is the right way to go. Reid's point about an
abstraction penalty is interesting, but I don't think we do ownership
transfers often enough to actually see a performance hit. (Of course, in the
non-transferring case we'd just pass the pointer, not a 'const
std::unique_ptr &' or anything.)

Jordan


On Jul 17, 2014, at 16:21 , David Blaikie <dblaikie at gmail.com> wrote:
> There seems to be some uncertainty about the use of smart pointers
> (previously OwningPtr, now std::unique_ptr and std::shared_ptr
> predominantly) in the LLVM project as a whole, so here's a thread to
> discuss/clarify/etc the project preferences/direction with regard to
> smart pointer usage.
> 
> For some context, see discussions in LLVM r212403 and Clang r213307.
> 
> The basic question here seems to be whether smart pointer ownership is
> a direction we want to take the LLVM project in general.
> 
> I've seen others contribute and have myself contributed many patches
> moving towards smart pointer ownership (both in the pre-C++11 days of
> OwningPtr, and much moreso in the post-C++11 world with
> std::unique_ptr and std::shared_ptr being usable inside containers, as
> return values, etc, allowing many more opportunities).
> 
> std::unique_ptr's been used in LLD as far back as r153620.
> std::unique_ptr appeared in LLVM shortly after the C++11 switch with
> Ahmed's work to migrate the project from OwningPtr to std::unique_ptr
> (starting with r202609 and ending with r211259). Originally OwningPtr
> was added in r45261.
> Something in the order of 60 changes across clang and LLVM mention
> unique_ptr in their subject and migrate various APIs to use unique_ptr
> for ownership. Many of which remove uses of explicit delete or helpers
> like DeleteContainerPointers (and removing explicit dtors in many of
> those cases).
> 
> Are people OK with/prefer the use of owning smart pointers in APIs?
> Are there places where you've found them to be too noisy/burdensome
> and would rather use raw pointers or some other abstraction? Would you
> prefer pre-commit review of such changes to adequately consider
> alternatives (such as?)?
> 
> Thanks,
> - David
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

On Fri, Jul 18, 2014 at 12:06 PM, Jordan Rose <jordan_rose at apple.com>
wrote:
> I don't have much to add here besides +1. I think using std::unique_ptr
> even for create* functions/methods is the right way to go.

+1 smart pointers here are a win in terms of safety and self-documentation.
I don't see why create* factories should be treated differently.

Eli



> Reid's point about an abstraction penalty is interesting, but I
don't
> think we do ownership transfers often enough to actually see a performance
> hit. (Of course, in the non-transferring case we'd just pass the
pointer,
> not a 'const std::unique_ptr &' or anything.)
>
> Jordan
>
>
> On Jul 17, 2014, at 16:21 , David Blaikie <dblaikie at gmail.com>
wrote:
>
> > There seems to be some uncertainty about the use of smart pointers
> > (previously OwningPtr, now std::unique_ptr and std::shared_ptr
> > predominantly) in the LLVM project as a whole, so here's a thread
to
> > discuss/clarify/etc the project preferences/direction with regard to
> > smart pointer usage.
> >
> > For some context, see discussions in LLVM r212403 and Clang r213307.
> >
> > The basic question here seems to be whether smart pointer ownership is
> > a direction we want to take the LLVM project in general.
> >
> > I've seen others contribute and have myself contributed many
patches
> > moving towards smart pointer ownership (both in the pre-C++11 days of
> > OwningPtr, and much moreso in the post-C++11 world with
> > std::unique_ptr and std::shared_ptr being usable inside containers, as
> > return values, etc, allowing many more opportunities).
> >
> > std::unique_ptr's been used in LLD as far back as r153620.
> > std::unique_ptr appeared in LLVM shortly after the C++11 switch with
> > Ahmed's work to migrate the project from OwningPtr to
std::unique_ptr
> > (starting with r202609 and ending with r211259). Originally OwningPtr
> > was added in r45261.
> > Something in the order of 60 changes across clang and LLVM mention
> > unique_ptr in their subject and migrate various APIs to use unique_ptr
> > for ownership. Many of which remove uses of explicit delete or helpers
> > like DeleteContainerPointers (and removing explicit dtors in many of
> > those cases).
> >
> > Are people OK with/prefer the use of owning smart pointers in APIs?
> > Are there places where you've found them to be too
noisy/burdensome
> > and would rather use raw pointers or some other abstraction? Would you
> > prefer pre-commit review of such changes to adequately consider
> > alternatives (such as?)?
> >
> > Thanks,
> > - David
> > _______________________________________________
> > cfe-dev mailing list
> > cfe-dev at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140718/8ee25883/attachment.html>