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 <[email protected]> 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 < > [email protected]> 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 <[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 > > > > > > > > > > > > > > >
