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 < tmarsh...@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 > <tarmstr...@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 > > >
