> On Feb. 17, 2014, 7:05 p.m., Benjamin Hindman wrote: > >
I had run some tests as part of https://reviews.apache.org/r/18173/ to observe RVO in gcc-4.8. // It appears that in both of these cases, gcc was already eliminating the copy. Try<list<string> > g1 = os::glob("/*"); // (1) Copy is elided, no copy occurs. const Try<list<string> >& g2 = os::glob("/*"); // (2) No copy, of course. Some of our code performs (2) as an attempt at optimization, however I'm seeing no copy / assignment of 'Try's in either case. Since the vast majority of our code is similar to (1). This seems inconsistent and a bit arbitrary. Based on this, whenever we were creating a Try from an rvalue, I removed the const& as I've found no benefit in terms of optimization, and (1) appears easier to read and understand. Even if gcc could not elide the copy, 'Try's move constructor can be used in these cases. Constructing 'Try's from lvalues is much less common, but in these cases I can see (2) being relevant. What are your thoughts on this pattern? > On Feb. 17, 2014, 7:05 p.m., Benjamin Hindman wrote: > > src/slave/state.cpp, line 128 > > <https://reviews.apache.org/r/18175/diff/1/?file=486928#file486928line128> > > > > Same for extending the lifetime here, did this not work!? > On Feb. 17, 2014, 7:05 p.m., Benjamin Hindman wrote: > > src/log/replica.cpp, line 721 > > <https://reviews.apache.org/r/18175/diff/1/?file=486922#file486922line721> > > > > What's the problem with keeping this a const &? - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18175/#review34658 ----------------------------------------------------------- On Feb. 16, 2014, 11:38 p.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18175/ > ----------------------------------------------------------- > > (Updated Feb. 16, 2014, 11:38 p.m.) > > > Review request for mesos, Benjamin Hindman, Jie Yu, Niklas Nielsen, and Vinod > Kone. > > > Bugs: MESOS-1008 > https://issues.apache.org/jira/browse/MESOS-1008 > > > Repository: mesos-git > > > Description > ------- > > See: https://reviews.apache.org/r/18173/ > > I also killed instances of our old pattern: > > const Try<T>& t = foo(); > > // Replaced with: > Try<T> t = foo(); > > I did this for two reasons: > 1. gcc is often already eliding the copy. > 2. This will be obviated when we add move constructors to Try, since foo() > is returning an rvalue. > > > Diffs > ----- > > src/log/replica.cpp 746d6c35c9255775ab6e70b0daf1dcecf63c16a0 > src/master/contender.cpp e3b0737195aba42c805a05c96b054cec1471b502 > src/messages/messages.hpp 98038c017a7d02c7c4f04f91c89ca56f566f707b > src/slave/containerizer/isolators/cgroups/cpushare.cpp > 989d3848343d4255c25796aa5be81dbb93a29b6e > src/slave/containerizer/isolators/cgroups/mem.cpp > a01e114c168b820e6c54af9257716b753940d510 > src/slave/slave.cpp 8ad955a71957421ae51e1becc4b06a4b6b091eb2 > src/slave/state.cpp 9af6c5b0582e972523028d226703070293b92f8b > src/tests/cgroups_tests.cpp f0dead7c09fe3cb4355c5b3b4603a2cb8c2cd3d5 > src/tests/mesos.cpp 98333a7d3a2c7c8004aacc43dda3add31d5b5487 > src/tests/values_tests.cpp c72dbc2ec05e12541c03196577dff3561bff08ed > src/tests/zookeeper_url_tests.cpp b143c539530e3319f603d9276ebf84dfecfcf6f2 > > Diff: https://reviews.apache.org/r/18175/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Ben Mahler > >
