You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Anand Mazumdar <an...@mesosphere.io> on 2015/06/09 17:56:28 UTC
Re: Review Request 35179: MESOS-1733 Variadic Path Join
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/
-----------------------------------------------------------
(Updated June 9, 2015, 3:56 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.
Might have some C++ style issues owing to this being my first commit here.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c
Diff: https://reviews.apache.org/r/35179/diff/
Testing
-------
make check + added some additional tests.
Thanks,
Anand Mazumdar
Re: Review Request 35179: MESOS-1733 Variadic Path Join
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/#review87584
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/35179/#comment139974>
not your code, but can you please get rid of the TABs?
- Marco Massenzio
On June 9, 2015, 3:56 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35179/
> -----------------------------------------------------------
>
> (Updated June 9, 2015, 3:56 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.
>
> Might have some C++ style issues owing to this being my first commit here.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c
>
> Diff: https://reviews.apache.org/r/35179/diff/
>
>
> Testing
> -------
>
> make check + added some additional tests.
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 35179: MESOS-1733 Variadic Path Join
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/#review87747
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35179]
All tests passed.
- Mesos ReviewBot
On June 12, 2015, 7: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, 7: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
>
>
Re: Review Request 35179: MESOS-1733 Variadic Path Join
Posted by Anand Mazumdar <ma...@gmail.com>.
> On June 15, 2015, 9:50 p.m., Cody Maloney wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 50
> > <https://reviews.apache.org/r/35179/diff/6/?file=984730#file984730line50>
> >
> > Looks like there is an accidental space addded after path1 here
My bad, Fixed.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/#review87978
-----------------------------------------------------------
On June 15, 2015, 10:45 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35179/
> -----------------------------------------------------------
>
> (Updated June 15, 2015, 10:45 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/tests/path_tests.cpp cf354125687e0f60b6d5b105f19d75e4436f21bf
>
> Diff: https://reviews.apache.org/r/35179/diff/
>
>
> Testing
> -------
>
> make check + added some additional tests.
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 35179: MESOS-1733 Variadic Path Join
Posted by Cody Maloney <co...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/#review87978
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/35179/#comment140389>
Looks like there is an accidental space addded after path1 here
- Cody Maloney
On June 15, 2015, 3:25 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35179/
> -----------------------------------------------------------
>
> (Updated June 15, 2015, 3:25 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/tests/path_tests.cpp cf354125687e0f60b6d5b105f19d75e4436f21bf
>
> Diff: https://reviews.apache.org/r/35179/diff/
>
>
> Testing
> -------
>
> make check + added some additional tests.
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 35179: MESOS-1733 Variadic Path Join
Posted by Michael Park <mc...@gmail.com>.
> On June 15, 2015, 10:57 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, lines 49-51
> > <https://reviews.apache.org/r/35179/diff/6/?file=984730#file984730line49>
> >
> > Not yours, but could you fix the formatting here to not wrap after `return` please?
> >
> > ```
> > return strings::remove(path1, "/", strings::SUFFIX) + "/" +
> > strings::remove(path2, "/", strings::PREFIX);
> > ```
Not sure why this got duplicated.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/#review87989
-----------------------------------------------------------
On June 15, 2015, 10:45 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35179/
> -----------------------------------------------------------
>
> (Updated June 15, 2015, 10:45 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/tests/path_tests.cpp cf354125687e0f60b6d5b105f19d75e4436f21bf
>
> Diff: https://reviews.apache.org/r/35179/diff/
>
>
> Testing
> -------
>
> make check + added some additional tests.
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 35179: MESOS-1733 Variadic Path Join
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/#review87989
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/35179/#comment140398>
Maybe `s/Default/Base/`?
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/35179/#comment140400>
Not yours, but could you fix the formatting here to not wrap after `return` please?
```
return strings::remove(path1, "/", strings::SUFFIX) + "/" +
strings::remove(path2, "/", strings::PREFIX);
```
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/35179/#comment140401>
Not yours, but could you fix the formatting here to not wrap after `return` please?
```
return strings::remove(path1, "/", strings::SUFFIX) + "/" +
strings::remove(path2, "/", strings::PREFIX);
```
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/35179/#comment140399>
`s/path1 ,/path1,`
- Michael Park
On June 15, 2015, 10:45 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35179/
> -----------------------------------------------------------
>
> (Updated June 15, 2015, 10:45 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/tests/path_tests.cpp cf354125687e0f60b6d5b105f19d75e4436f21bf
>
> Diff: https://reviews.apache.org/r/35179/diff/
>
>
> Testing
> -------
>
> make check + added some additional tests.
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 35179: MESOS-1733 Variadic Path Join
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/#review87999
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35179]
All tests passed.
- Mesos ReviewBot
On June 15, 2015, 11:23 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35179/
> -----------------------------------------------------------
>
> (Updated June 15, 2015, 11:23 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/tests/path_tests.cpp cf354125687e0f60b6d5b105f19d75e4436f21bf
>
> Diff: https://reviews.apache.org/r/35179/diff/
>
>
> Testing
> -------
>
> make check + added some additional tests.
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 35179: MESOS-1733 Variadic Path Join
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/#review88059
-----------------------------------------------------------
Ship it!
Ship It!
- Michael Park
On June 15, 2015, 11:23 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35179/
> -----------------------------------------------------------
>
> (Updated June 15, 2015, 11:23 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/tests/path_tests.cpp cf354125687e0f60b6d5b105f19d75e4436f21bf
>
> Diff: https://reviews.apache.org/r/35179/diff/
>
>
> Testing
> -------
>
> make check + added some additional tests.
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 35179: MESOS-1733 Variadic Path Join
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/#review88312
-----------------------------------------------------------
Ship it!
Looks great! I'll get this committed tonight
- Adam B
On June 15, 2015, 4:23 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35179/
> -----------------------------------------------------------
>
> (Updated June 15, 2015, 4:23 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/tests/path_tests.cpp cf354125687e0f60b6d5b105f19d75e4436f21bf
>
> Diff: https://reviews.apache.org/r/35179/diff/
>
>
> Testing
> -------
>
> make check + added some additional tests.
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 35179: MESOS-1733 Variadic Path Join
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/
-----------------------------------------------------------
(Updated June 15, 2015, 11:23 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 (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c
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
Re: Review Request 35179: MESOS-1733 Variadic Path Join
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/
-----------------------------------------------------------
(Updated June 15, 2015, 10:45 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 (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c
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
Re: Review Request 35179: MESOS-1733 Variadic Path Join
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/#review87933
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35179]
All tests passed.
- Mesos ReviewBot
On June 15, 2015, 3:25 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35179/
> -----------------------------------------------------------
>
> (Updated June 15, 2015, 3:25 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/tests/path_tests.cpp cf354125687e0f60b6d5b105f19d75e4436f21bf
>
> Diff: https://reviews.apache.org/r/35179/diff/
>
>
> Testing
> -------
>
> make check + added some additional tests.
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 35179: MESOS-1733 Variadic Path Join
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/
-----------------------------------------------------------
(Updated June 15, 2015, 3:25 p.m.)
Review request for mesos, Adam B and Cody Maloney.
Changes
-------
Address Adam's review comments.
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 (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c
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
Re: Review Request 35179: MESOS-1733 Variadic Path Join
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 35179: MESOS-1733 Variadic Path Join
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/
-----------------------------------------------------------
(Updated June 12, 2015, 7:04 p.m.)
Review request for mesos, Adam B and Cody Maloney.
Changes
-------
Amended commit to include email info to make review-bot happy.
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 (updated)
-----
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
Re: Review Request 35179: MESOS-1733 Variadic Path Join
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/#review87739
-----------------------------------------------------------
Bad patch!
Reviews applied: [35179]
Failed command: ./support/apply-review.sh -n -r 35179
Error:
2015-06-12 18:25:44 URL:https://reviews.apache.org/r/35179/diff/raw/ [5407/5407] -> "35179.patch" [1]
'fullname' was not found
'email' was not found
Successfully applied: MESOS-1733 Variadic Path Join
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.
Review: https://reviews.apache.org/r/35179
fatal: empty ident name (for <>) not allowed
Failed to commit patch
- Mesos ReviewBot
On June 12, 2015, 5:17 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, 5:17 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
>
>
Re: Review Request 35179: MESOS-1733 Variadic Path Join
Posted by Anand Mazumdar <an...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/
-----------------------------------------------------------
(Updated June 12, 2015, 5:17 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 (updated)
-------
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 (updated)
-----
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
Re: Review Request 35179: MESOS-1733 Variadic Path Join
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/#review87696
-----------------------------------------------------------
Bad patch!
Reviews applied: [35179]
Failed command: ./support/apply-review.sh -n -r 35179
Error:
2015-06-12 10:55:57 URL:https://reviews.apache.org/r/35179/diff/raw/ [1396/1396] -> "35179.patch" [1]
error: patch failed: 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp:17
error: 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On June 12, 2015, 10:32 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35179/
> -----------------------------------------------------------
>
> (Updated June 12, 2015, 10:32 a.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.
>
> Might have some C++ style issues owing to this being my first commit here.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c
>
> Diff: https://reviews.apache.org/r/35179/diff/
>
>
> Testing
> -------
>
> make check + added some additional tests.
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 35179: MESOS-1733 Variadic Path Join
Posted by Anand Mazumdar <an...@mesosphere.io>.
> On June 12, 2015, 11:15 a.m., Adam B wrote:
> > Looks like Cody already made path::join() variadic in commit b08fccf8f5ea325b8c38055b5f2c03509744dd9b "Switched path::join() to be variadic". How is your patch an improvement? What problem is it solving?
> >
> > Please remove the "Might have some C++ style issues..." line in your description, since the apply-review script uses the description to create the commit message. You can add any comments to reviewers in the Testing section, since that is ignored by apply-review.
> >
> > Your Testing Done section says that you "added some additional tests", but I don't see them in this patch. Looks like they were in a previous revision, but removed. Please add them back or update the Testing section.
> >
> > ReviewBot says the patch doesn't apply cleanly, so please rebase.
- Cody's patch "b08fccf8f5ea325b8c38055b5f2c03509744dd9b" tried to also optimize how we "append" the path-separator "/" by making trimming calls to remove/add them recusively during the join(...) function calls. This led to some errors in the implementation and made it deviate from the earlier join(...) behavior i.e. prior to it being variadic. This patch takes a much simpler route at tackling the issue by just making the function variadic minus the trimming optimizations ( they don't even buy us much anyways in terms of optimization ). Hence, this patch essentially tries to make the join(...) variadic by trying to keep the behavior as close to the previous one as possible.
- Removed the "might have some C++ style issues..." line from description.
- Added the deleted "additional tests". My bad, I was essentially submitting diffs based on Cody's comments without rebasing but just the "new" diff that addressed the comments. Putting them back now.
- Rebased from master, so ReviewBot hopefully should not complain anymore.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/#review87699
-----------------------------------------------------------
On June 12, 2015, 5:17 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, 5:17 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
>
>
Re: Review Request 35179: MESOS-1733 Variadic Path Join
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/#review87699
-----------------------------------------------------------
Looks like Cody already made path::join() variadic in commit b08fccf8f5ea325b8c38055b5f2c03509744dd9b "Switched path::join() to be variadic". How is your patch an improvement? What problem is it solving?
Please remove the "Might have some C++ style issues..." line in your description, since the apply-review script uses the description to create the commit message. You can add any comments to reviewers in the Testing section, since that is ignored by apply-review.
Your Testing Done section says that you "added some additional tests", but I don't see them in this patch. Looks like they were in a previous revision, but removed. Please add them back or update the Testing section.
ReviewBot says the patch doesn't apply cleanly, so please rebase.
- Adam B
On June 12, 2015, 3:32 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35179/
> -----------------------------------------------------------
>
> (Updated June 12, 2015, 3:32 a.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.
>
> Might have some C++ style issues owing to this being my first commit here.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c
>
> Diff: https://reviews.apache.org/r/35179/diff/
>
>
> Testing
> -------
>
> make check + added some additional tests.
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 35179: MESOS-1733 Variadic Path Join
Posted by Anand Mazumdar <an...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35179/
-----------------------------------------------------------
(Updated June 12, 2015, 3:32 a.m.)
Review request for mesos, Adam B and Cody Maloney.
Changes
-------
Removed "Depends on" self
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.
Might have some C++ style issues owing to this being my first commit here.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c
Diff: https://reviews.apache.org/r/35179/diff/
Testing
-------
make check + added some additional tests.
Thanks,
Anand Mazumdar