> On Feb. 17, 2014, 7:05 p.m., Benjamin Hindman wrote:
> >
>
> Ben Mahler 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?
After chatting with a few folks, we felt that both should be considered valid.
However, (1) should be preferred for our Option, Try, Result, Future 'wrapper'
types, because:
-It is cleaner and slightly easier to read.
-The copy is often elided by gcc (for inline and other visible functions),
and when not elided, the copy will become a move with C++11.
-The vast majority of our code uses this form, we should remain consistent.
(2) should be used for other types when there is a desire to _definitely_
eliminate a copy, or when const semantics are desired. It's still not clear to
me whether we should apply different rules to {Option,Try,Result,Future} vs
{string,T,...}.
Accordingly, I've kept my removal of a few instances of case (2) in order to
make the code cleaner and more consistent.
Definitely still room for discussion here, something we should bring up in the
style guide for sure!
> 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!?
>
> Ben Mahler wrote:
>
- 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
>
>
