-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/#review87917
-----------------------------------------------------------


Looks good. Besides some style nits, I'd also like you to remove the 
single-argument join and put the REQUIRE macro into a separate patch.


3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/35179/#comment140341>

    Also #include <utility> // std::forward



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/35179/#comment140340>

    Why is this included? I don't think you're actually using enable_if in this 
file anymore.



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/35179/#comment140331>

    Remove blank line



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/35179/#comment140332>

    Why do we need this single-element join version? Seems like the two-element 
version would be the sensible base case.



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/35179/#comment140333>

    This can fit on one line (<=80 char), so let's do so.



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/35179/#comment140342>

    Remove tab/indentation please



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/35179/#comment140343>

    Should only indent 2 spaces



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/35179/#comment140334>

    Two blank lines between function implementations, please.



3rdparty/libprocess/3rdparty/stout/include/stout/require.hpp
<https://reviews.apache.org/r/35179/#comment140335>

    This macro is unused in this patch. While it may be valuable, let's split 
it off into a separate patch to be reviewed separately. Ideally we would 
introduce the macro along with another patch that actually uses it.



3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp
<https://reviews.apache.org/r/35179/#comment140339>

    Would it be worthwhile to also test:
    path::join("ab", "/", "/")
    path::join("/", "", "/ab")



3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp
<https://reviews.apache.org/r/35179/#comment140338>

    I don't think a single-argument join makes any sense. We didn't support one 
before, and I would expect such a call to fail at compile-time. Please remove.


- Adam B


On June 12, 2015, 12:04 p.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35179/
> -----------------------------------------------------------
> 
> (Updated June 12, 2015, 12:04 p.m.)
> 
> 
> Review request for mesos, Adam B and Cody Maloney.
> 
> 
> Bugs: MESOS-1733
>     https://issues.apache.org/jira/browse/MESOS-1733
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This change takes an un-complicated/naive route ( no trimming of values etc ) 
> at making path::join(...) variadic mainly in order to preserve the earlier 
> over-loaded join functionality.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
> d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c 
>   3rdparty/libprocess/3rdparty/stout/include/stout/require.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
> cf354125687e0f60b6d5b105f19d75e4436f21bf 
> 
> Diff: https://reviews.apache.org/r/35179/diff/
> 
> 
> Testing
> -------
> 
> make check + added some additional tests.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Reply via email to