> On Dec. 15, 2014, 9:29 p.m., Ben Mahler wrote: > > include/mesos/resources.hpp, lines 211-214 > > <https://reviews.apache.org/r/29018/diff/1/?file=791218#file791218line211> > > > > To keep consistent with the rest of the code base, couldn't you have > > done the following? > > > > ``` > > template <typename T> > > void add(const T& t) > > { > > transformations.push_back(new T(t)); > > } > > ``` > > > > With unique_ptr, we're requiring that ownership be transferred if the > > caller is holding the transformation, which seems a bit odd for our library > > to impose on the caller: > > > > ``` > > // The caller cannot keep ownership of t. > > std::unique_ptr<Transformation> t(new DiskTransformation(...)); > > composite.add(std::move(t)); > > > > // Ok. > > composite.add(std::unique_ptr<Transformation>(new > > DiskTransformation(...)); > > ``` > > > > Could we keep the const argument semantics for now for consistency? > > Then the caller doesn't have to use 'new' as well. Thoughts? > > Dominic Hamon wrote: > That's exactly the point of std::unique_ptr - a library is finally able > to inform the caller that they're taking responsibility for the object and > the memory associated with it. I don't see that as odd. > > Can you describe a use case where a caller might want to retain a > reference to the transformation? > > Jie Yu wrote: > OK, since we haven't finalized the unique_ptr usage in the style guide, > I'll use your suggestion and add a TODO here.
Thanks Jie and Dominic! This could very well be a great place to leverage unique_ptr, but as jie said let's take this example to the thread :) The point that we'll want to consider there is that function calls with value semantics are generally easier to reason about: ```cpp CompositeTransformation composite; composite.add(DiskTransformation(disk)); FooBarTransformation transformation; ...; composite.add(transformation); ``` The caller here only has to reason about values. The callee is responsible for taking a copy if it's needed. Here we have to reason about memory ownership: ```cpp CompositeTransformation composite; composite.add(unique_ptr<Transformation>(new DiskTransformation(disk))); unique_ptr<Transformation> transformation(new FooBarTransformation()); ...; composite.add(std::move(transformation)); // Also consider: transformation->...; // Yikes! As far as I can tell, the compiler won't warn about this? ``` Another point to bring up in the summary from that thread (as it's getting a bit long), is whether compilers warn about using moved objects, doesn't seem like it? http://stackoverflow.com/questions/14552690/warn-if-accessing-moved-object-in-c11 - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29018/#review65121 ----------------------------------------------------------- On Dec. 16, 2014, 9:10 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29018/ > ----------------------------------------------------------- > > (Updated Dec. 16, 2014, 9:10 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-2030 > https://issues.apache.org/jira/browse/MESOS-2030 > > > Repository: mesos-git > > > Description > ------- > > Added abstraction for resources transformation. Split from > https://reviews.apache.org/r/28720/. > > > Diffs > ----- > > include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc > src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92 > > Diff: https://reviews.apache.org/r/29018/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
