mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joris Van Remoortere" <joris.van.remoort...@gmail.com>
Subject Re: Review Request 34529: Add non-const reference version of Option<T>::get.
Date Fri, 29 May 2015 20:21:48 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message