----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18175/#review34658 -----------------------------------------------------------
src/log/replica.cpp <https://reviews.apache.org/r/18175/#comment64852> What's the problem with keeping this a const &? src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/18175/#comment64857> Once we start using the Option returned from hashmap I think we should restructure this code: Option<T> user = stat.get().get("user"); Option<T> system = stat.get().get("system"); if (user.isSome() && system.isSome()) { result.set_cpus_user_time_secs( (double) user.get() / (double) ticks); result.set_cpus_system_time_secs( (double) system.get() / (double) ticks); } src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/18175/#comment64858> Then these could become: Option<T> nr_periods = stat.get().get("nr_periods"); if (nr_periods.isSome()) { result.set_cpus_nr_periods(nr_periods.get()); } src/slave/slave.cpp <https://reviews.apache.org/r/18175/#comment64853> Why not keep the const &? You should be able to extend the lifetime of the temporary via the const &. src/slave/state.cpp <https://reviews.apache.org/r/18175/#comment64854> Same for extending the lifetime here, did this not work!? src/slave/state.cpp <https://reviews.apache.org/r/18175/#comment64855> This one should definitely work, allowing the compiler to do RVO. src/slave/state.cpp <https://reviews.apache.org/r/18175/#comment64856> No RVO here either? Perhaps you preferred the code without the const &? ;) I'll stop commenting on these now ... src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/18175/#comment64859> Too bad we don't have an EXPECT_SOME_GT! ;) - Benjamin Hindman 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 > >
