mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam B" <a...@mesosphere.io>
Subject Re: Review Request 35179: MESOS-1733 Variadic Path Join
Date Mon, 15 Jun 2015 13:23:04 GMT

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


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