You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by Tim Armstrong <ta...@cloudera.com.INVALID> on 2018/07/05 23:38:26 UTC
boost::scoped_ptr vs std::unique_ptr
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
Re: boost::scoped_ptr vs std::unique_ptr
Posted by Jim Apple <jb...@cloudera.com.INVALID>.
Thank you for speaking up, Zoltan!
On Fri, Jul 6, 2018 at 1:26 PM, Zoltan Borok-Nagy <
boroknagyz@cloudera.com.invalid> wrote:
> Yeah I see that, no worries.
> I only wanted to draw some attention to it, so we might not forget it in
> the future.
>
> - Zoltan
>
> On Fri, Jul 6, 2018 at 5:57 PM Tim Armstrong
> <ta...@cloudera.com.invalid> wrote:
>
> > That's a fair observation Zoltan, we should avoid doing that. I would
> have
> > definitely waited longer if it was a more consequential or irreversible
> > decision, but I think even for more minor things there's no reason not to
> > wait longer.
> >
> > On Fri, Jul 6, 2018 at 4:03 AM, Zoltan Borok-Nagy <
> > boroknagyz@cloudera.com.invalid> wrote:
> >
> > > 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 <jbapple@cloudera.com.invalid
> >
> > > wrote:
> > >
> > > > SGTM
> > > >
> > > > On Thu, Jul 5, 2018 at 6:13 PM, Tim Armstrong <
> > > > tarmstrong@cloudera.com.invalid> 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
> > <todd@cloudera.com.invalid
> > > >
> > > > > 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
> > > <jbapple@cloudera.com.invalid
> > > > >
> > > > > > 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
> > > > > > <sailesh@cloudera.com.invalid
> > > > > > > >
> > > > > > > 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 <
> > > > > > > > tmarshall@cloudera.com.invalid> 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
> > > > > > > > > <ta...@cloudera.com.invalid> 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
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: boost::scoped_ptr vs std::unique_ptr
Posted by Zoltan Borok-Nagy <bo...@cloudera.com.INVALID>.
Yeah I see that, no worries.
I only wanted to draw some attention to it, so we might not forget it in
the future.
- Zoltan
On Fri, Jul 6, 2018 at 5:57 PM Tim Armstrong
<ta...@cloudera.com.invalid> wrote:
> That's a fair observation Zoltan, we should avoid doing that. I would have
> definitely waited longer if it was a more consequential or irreversible
> decision, but I think even for more minor things there's no reason not to
> wait longer.
>
> On Fri, Jul 6, 2018 at 4:03 AM, Zoltan Borok-Nagy <
> boroknagyz@cloudera.com.invalid> wrote:
>
> > 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 <jb...@cloudera.com.invalid>
> > wrote:
> >
> > > SGTM
> > >
> > > On Thu, Jul 5, 2018 at 6:13 PM, Tim Armstrong <
> > > tarmstrong@cloudera.com.invalid> 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
> <todd@cloudera.com.invalid
> > >
> > > > 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
> > <jbapple@cloudera.com.invalid
> > > >
> > > > > 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
> > > > > <sailesh@cloudera.com.invalid
> > > > > > >
> > > > > > 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 <
> > > > > > > tmarshall@cloudera.com.invalid> 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
> > > > > > > > <ta...@cloudera.com.invalid> 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
> > > > >
> > > >
> > >
> >
>
Re: boost::scoped_ptr vs std::unique_ptr
Posted by Tim Armstrong <ta...@cloudera.com.INVALID>.
That's a fair observation Zoltan, we should avoid doing that. I would have
definitely waited longer if it was a more consequential or irreversible
decision, but I think even for more minor things there's no reason not to
wait longer.
On Fri, Jul 6, 2018 at 4:03 AM, Zoltan Borok-Nagy <
boroknagyz@cloudera.com.invalid> wrote:
> 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 <jb...@cloudera.com.invalid>
> wrote:
>
> > SGTM
> >
> > On Thu, Jul 5, 2018 at 6:13 PM, Tim Armstrong <
> > tarmstrong@cloudera.com.invalid> 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 <todd@cloudera.com.invalid
> >
> > > 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
> <jbapple@cloudera.com.invalid
> > >
> > > > 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
> > > > <sailesh@cloudera.com.invalid
> > > > > >
> > > > > 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 <
> > > > > > tmarshall@cloudera.com.invalid> 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
> > > > > > > <ta...@cloudera.com.invalid> 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
> > > >
> > >
> >
>
Re: boost::scoped_ptr vs std::unique_ptr
Posted by Zoltan Borok-Nagy <bo...@cloudera.com.INVALID>.
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 <jb...@cloudera.com.invalid>
wrote:
> SGTM
>
> On Thu, Jul 5, 2018 at 6:13 PM, Tim Armstrong <
> tarmstrong@cloudera.com.invalid> 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 <to...@cloudera.com.invalid>
> > 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 <jbapple@cloudera.com.invalid
> >
> > > 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
> > > <sailesh@cloudera.com.invalid
> > > > >
> > > > 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 <
> > > > > tmarshall@cloudera.com.invalid> 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
> > > > > > <ta...@cloudera.com.invalid> 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
> > >
> >
>
Re: boost::scoped_ptr vs std::unique_ptr
Posted by Jim Apple <jb...@cloudera.com.INVALID>.
SGTM
On Thu, Jul 5, 2018 at 6:13 PM, Tim Armstrong <
tarmstrong@cloudera.com.invalid> 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 <to...@cloudera.com.invalid>
> 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 <jb...@cloudera.com.invalid>
> > 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
> > <sailesh@cloudera.com.invalid
> > > >
> > > 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 <
> > > > tmarshall@cloudera.com.invalid> 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
> > > > > <ta...@cloudera.com.invalid> 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
> >
>
Re: boost::scoped_ptr vs std::unique_ptr
Posted by Tim Armstrong <ta...@cloudera.com.INVALID>.
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 <to...@cloudera.com.invalid>
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 <jb...@cloudera.com.invalid>
> 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
> <sailesh@cloudera.com.invalid
> > >
> > 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 <
> > > tmarshall@cloudera.com.invalid> 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
> > > > <ta...@cloudera.com.invalid> 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
>
Re: boost::scoped_ptr vs std::unique_ptr
Posted by Todd Lipcon <to...@cloudera.com.INVALID>.
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 <jb...@cloudera.com.invalid>
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 <sailesh@cloudera.com.invalid
> >
> 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 <
> > tmarshall@cloudera.com.invalid> 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
> > > <ta...@cloudera.com.invalid> 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
Re: boost::scoped_ptr vs std::unique_ptr
Posted by Jim Apple <jb...@cloudera.com.INVALID>.
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 <sa...@cloudera.com.invalid>
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 <
> tmarshall@cloudera.com.invalid> 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
> > <ta...@cloudera.com.invalid> 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
> > >
> >
>
Re: boost::scoped_ptr vs std::unique_ptr
Posted by Sailesh Mukil <sa...@cloudera.com.INVALID>.
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 <
tmarshall@cloudera.com.invalid> 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
> <ta...@cloudera.com.invalid> 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
> >
>
Re: boost::scoped_ptr vs std::unique_ptr
Posted by Thomas Tauber-Marshall <tm...@cloudera.com.INVALID>.
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
<ta...@cloudera.com.invalid> 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
>