Hi,
I'm also in favor of unique_ptr and maybe const unique_ptr.
But I want to point out that the duration of the whole discussion was 1.5
hours.
Maybe we could keep such threads open at least 24 hours to let anyone in
the globe weigh in.
BR,
Zoltan
On Fri, Jul 6, 2018 at 3:31 AM Jim Apple <[email protected]>
wrote:
> SGTM
>
> On Thu, Jul 5, 2018 at 6:13 PM, Tim Armstrong <
> [email protected]> wrote:
>
> > Sounds like unique_ptr is preferred then going forward. I updated the
> wiki
> > page.
> >
> > > Fwiw, I was under the impression from talking with people in the past
> > that
> > > we were already trying to make this move, and the
> > > PartitionedAggregationNode refactor that just went in made the switch
> to
> > > unique_ptr, though no one commented on it in the review.
> > Yeah it sounds like there are two camps - people wanting to use
> unique_ptr
> > and people who don't mind scoped_ptr but are apathetic about it.
> >
> > > I suspect we could also make own own immobile_ptr with minimal effort,
> > > thereby both making the difference more visible and reducing boost
> > > dependence.
> > I'd thought about this too and I'm not sure that it's worth doing
> something
> > non-standard that new contributors will have to learn.
> >
> >
> > On Thu, Jul 5, 2018 at 5:27 PM, Todd Lipcon <[email protected]>
> > wrote:
> >
> > > Definitely in favor.
> > >
> > > Personally I never found the "this pointer isn't movable" to be a
> > > worthwhile distinction. With unique_ptr you need to pretty explicitly
> > move
> > > it using std::move, so you don't get "accidental" moves like you used
> to
> > > with std::auto_ptr.
> > >
> > > Looking briefly at Kudu we have 129 unique_ptr members and only 7 of
> them
> > > are marked const, so at least we haven't found that a particularly
> useful
> > > pattern.
> > >
> > > -Todd
> > >
> > > On Thu, Jul 5, 2018 at 5:23 PM, Jim Apple <[email protected]
> >
> > > wrote:
> > >
> > > > I suspect we could also make own own immobile_ptr with minimal
> effort,
> > > > thereby both making the difference more visible and reducing boost
> > > > dependence.
> > > >
> > > > On Thu, Jul 5, 2018 at 5:17 PM, Sailesh Mukil
> > > <[email protected]
> > > > >
> > > > wrote:
> > > >
> > > > > I'm in favor.
> > > > >
> > > > > Since the main distinction is that a unique_ptr is moveable,
> whereas
> > a
> > > > > scoped_ptr is not, we should just make sure that we do our due
> > > diligence
> > > > > during code reviews so that we catch those cases.
> > > > >
> > > > > Also, making a unique_ptr const disallows moving it, since the move
> > > > > constructor takes a non-const unique_ptr container. It probably
> won't
> > > > work
> > > > > in all places, but we could enforce that in certain parts of the
> > code.
> > > > >
> > > > > On Thu, Jul 5, 2018 at 4:49 PM, Thomas Tauber-Marshall <
> > > > > [email protected]> wrote:
> > > > >
> > > > > > I'm definitely in favor of using more standard c++ to reduce both
> > > > > confusion
> > > > > > and our reliance on boost, especially as I suspect a lot of
> people
> > > (eg.
> > > > > me)
> > > > > > don't know the subtle difference between scoped_ptr and
> unique_ptr
> > > off
> > > > > the
> > > > > > top of their head anyways.
> > > > > >
> > > > > > Fwiw, I was under the impression from talking with people in the
> > past
> > > > > that
> > > > > > we were already trying to make this move, and the
> > > > > > PartitionedAggregationNode refactor that just went in made the
> > switch
> > > > to
> > > > > > unique_ptr, though no one commented on it in the review.
> > > > > >
> > > > > > On Thu, Jul 5, 2018 at 4:39 PM Tim Armstrong
> > > > > > <[email protected]> wrote:
> > > > > >
> > > > > > > I was just talking with Michael Ho on a review about this
> > > > > > > https://gerrit.cloudera.org/#/c/10810/7/be/src/exec/scan-
> > > node.h@271
> > > > > > >
> > > > > > > For a while we've continued using scoped_ptr in some places
> > because
> > > > it
> > > > > > > supports a smaller set of operators and implies that the
> pointer
> > > > isn't
> > > > > > > movable. See
> > > > > > >
> > > > > > > https://cwiki.apache.org/confluence/display/IMPALA/
> > > > > > Resource+Management+Best+Practices+in+Impala
> > > > > > > .
> > > > > > >
> > > > > > > I don't think we're consistently following this pattern and it
> > > seems
> > > > to
> > > > > > > cause some confusion about what the best practice is,
> > particularly
> > > > for
> > > > > > > people coming from other code bases. I personally like the
> > > > distinction,
> > > > > > but
> > > > > > > I don't feel that strongly about it.
> > > > > > >
> > > > > > > What do people think? Should we continue using scoped_ptr or
> move
> > > > away
> > > > > > from
> > > > > > > it. There is already a JIRA to make the change but we haven't
> > done
> > > it
> > > > > > > because of the above reasons:
> > > > > > > https://issues.apache.org/jira/browse/IMPALA-3444
> > > > > > >
> > > > > > > - Tim
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> > >
> > >
> > > --
> > > Todd Lipcon
> > > Software Engineer, Cloudera
> > >
> >
>
