You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by Henry Robinson <he...@cloudera.com> on 2016/09/01 00:24:11 UTC

Re: scoped_ptr -> unique_ptr?

On 31 August 2016 at 11:55, Marcel Kornacker <ma...@cloudera.com> wrote:

> Generally, we want to move away from implicitly-managed memory (d'tor
> frees) to explicitly, centrally managed memory (in MemPools, and
> d'tors become no-ops).
>

Can you clarify this a bit - particularly how a d'tor becomes a no-op?

For example, if the MemPool is owned by the same object that has the
members that we would otherwise use a smart pointer for, there's no
difference in allocation behaviour. The d'tor will destroy the MemPool just
like it currently destroys scoped_ptrs.

Is the intention to do explicit release of memory back to the MemPool -
i.e. completely manual memory management?



>
> With that in mind, I would limit the amount of effort spent on
> changing from one implicitly-managed model to another.
>
> That said, I'm in favor of doing the scoped_/unique_ptr replacement in
> the current codebase where it is necessary to move a reference. I'm
> not sure where that would currently apply.
>
> On Wed, Aug 31, 2016 at 11:30 AM, Tim Armstrong <ta...@cloudera.com>
> wrote:
> > I don't feel that strongly about it but, but I think
> scoped_ptr/unique_ptr
> > is a useful distinction to document that the pointer isn't movable.
> >
> > I think it contains info both ways: if it's a unique_ptr it means we
> intend
> > to (sometimes) transfer it, but if it's a scoped_ptr its lifetime is
> > strictly bounded by the owner.
> >
> > I agree it's hard to accidentally transfer something but I think if we
> use
> > the same type it's much easier to violate the implicit memory lifetime
> > assumptions in a bit of code without warning.
> >
> > E.g. if you have class Parent that contains a scoped_ptr<Child> child_,
> > then it's always safe for child_ to store a Parent* reference. But if you
> > change it to unique_ptr<Child> then it's no longer obvious that this is
> > safe without auditing references to child_ to make sure it isn't
> release()d
> > or moved.
> >
> > On Wed, Aug 31, 2016 at 11:24 AM, Jim Apple <jb...@cloudera.com>
> wrote:
> >
> >> I'm convinced. +1 to moving to ::std::unique_ptr.
> >>
> >> On Wed, Aug 31, 2016 at 11:22 AM, Henry Robinson <he...@cloudera.com>
> >> wrote:
> >> > On 31 August 2016 at 11:16, Jim Apple <jb...@cloudera.com> wrote:
> >> >
> >> >> Why should we reduce our boost dependency?
> >> >>
> >> >
> >> > Boost will sometimes break subtly (or unsubtly by changing APIs)
> between
> >> > versions, is often not as well tested as stdlib implementations and
> does
> >> > not have a standard. If there are reasonable std:: implementations of
> >> > boost:: primitives, experience has shown it's usually a good idea to
> opt
> >> > for the std
> >> >
> >> >
> >> >>
> >> >> Do you think there are places where scoped_ptr is used now where you
> >> >> would want to keep it indefinitely if it were part of the standard
> and
> >> >> not part of boost?
> >> >>
> >> >
> >> > No, but I can't say I've audited every location. For our typical
> uses, I
> >> > don't see a disadvantage to unique_ptr.
> >> >
> >> >
> >> >
> >> >>
> >> >> On Wed, Aug 31, 2016 at 10:47 AM, Henry Robinson <he...@apache.org>
> >> wrote:
> >> >> > We use boost::scoped_ptr everywhere to handle scope-owned
> >> heap-allocated
> >> >> > objects. C++11 has std::unique_ptr for this. I'd like to get a
> >> decision
> >> >> on
> >> >> > whether we should start standardising on unique_ptr. This is
> >> particularly
> >> >> > relevant for new code - should I call it out in code review?
> >> >> >
> >> >> > The most significant difference is that unique_ptr is moveable,
> which
> >> >> means
> >> >> > it can be used in collections (good!). It also means that badly
> >> written
> >> >> > code can allow scope-owned objects to escape their scope:
> >> >> >
> >> >> > private:
> >> >> >   unique_ptr<Foo> foo_;
> >> >> >
> >> >> > public:
> >> >> >   unique_ptr<Foo> get_foo() { return move(foo_); }
> >> >> >
> >> >> > or worse:
> >> >> >
> >> >> >   Foo* get_foo() { return foo_.release(); }
> >> >> >
> >> >> > In both cases you have to be quite explicit about the decision to
> >> yield
> >> >> > ownership of the owned object, and it seems to me that we should
> catch
> >> >> this
> >> >> > in code review.
> >> >> >
> >> >> > Since using unique_ptr in collections is so useful, and reducing
> our
> >> >> boost
> >> >> > dependency is generally worthwhile, I'm very much in favour of
> moving
> >> to
> >> >> > unique_ptr for future code, and at some point porting all the
> current
> >> >> > scoped_ptr to unique_ptr.
> >> >> >
> >> >> > What do you think?
> >> >> >
> >> >> > Henry
> >> >>
> >> >
> >> >
> >> >
> >> > --
> >> > Henry Robinson
> >> > Software Engineer
> >> > Cloudera
> >> > 415-994-6679
> >>
>



-- 
Henry Robinson
Software Engineer
Cloudera
415-994-6679