----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34529/#review85792 -----------------------------------------------------------
Ship it! Hey Mark, Thanks for your work! I left some comments, but this looks good to me once you fix them. Will you be making a follow up review that gets rid of the cases you used as justification for this change? Joris 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp <https://reviews.apache.org/r/34529/#comment137569> Could you please elaborate why we are adding this? I can't tell the difference between this and the function above. IMO if we want to change the name then we should refactor the original function as opposed to having 2 copies. If we did have 2 versions of the same function, would it make more sense to delegate one of them to the other (as we did with the other operators added in this patch)? I think this patch can get committed if we take this out. Do you want to pull this extra function (and tests) into a seperate patch to move the discussion and let the original JIRA get resolved? :-) 3rdparty/libprocess/3rdparty/stout/tests/option_tests.cpp <https://reviews.apache.org/r/34529/#comment137575> Please have 2 new lines between tests. (add 1) 3rdparty/libprocess/3rdparty/stout/tests/option_tests.cpp <https://reviews.apache.org/r/34529/#comment137572> the `&` binds to the type per our style guide: `const string& constReference` Here and below. 3rdparty/libprocess/3rdparty/stout/tests/option_tests.cpp <https://reviews.apache.org/r/34529/#comment137573> We put the expected value as the first argument, and the one we are testing as the second. This allows the tests to print out the right arguments if the test fails. Here and below. - Joris Van Remoortere On May 27, 2015, 6:40 a.m., Mark Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34529/ > ----------------------------------------------------------- > > (Updated May 27, 2015, 6:40 a.m.) > > > Review request for mesos and Joris Van Remoortere. > > > Bugs: MESOS-2716 > https://issues.apache.org/jira/browse/MESOS-2716 > > > Repository: mesos > > > Description > ------- > > Add non-const reference version of Option<T>::get. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp > ea79b501d9ed7b7da9636ce9c9c590738a586993 > 3rdparty/libprocess/3rdparty/stout/tests/option_tests.cpp > 7ae3b8ffc5df7f8442e72b1a10d50c3f5c373d8c > > Diff: https://reviews.apache.org/r/34529/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Mark Wang > >
