You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Till Toenshoff <to...@me.com> on 2015/05/17 21:46:14 UTC
Re: Review Request 34256: Added Path::dirname() and Path::basename().
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34256/
-----------------------------------------------------------
(Updated May 17, 2015, 7:46 p.m.)
Review request for mesos and Cody Maloney.
Changes
-------
Moved implementations into Path.
Summary (updated)
-----------------
Added Path::dirname() and Path::basename().
Bugs: MESOS-1303
https://issues.apache.org/jira/browse/MESOS-1303
Repository: mesos-incubating
Description (updated)
-------
Introducing Path::dirname() and Path::basename() as a thread safe replacement of the respective system functions. Also contains new tests covering corner cases.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df650
3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf35412
Diff: https://reviews.apache.org/r/34256/diff/
Testing (updated)
-------
make check (including new tests).
Result comparison to match ::dirname and ::basename on interesting cases.
Thanks,
Till Toenshoff
Re: Review Request 34256: Added Path::dirname() and Path::basename().
Posted by Till Toenshoff <to...@me.com>.
> On May 19, 2015, 8:07 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp, line 29
> > <https://reviews.apache.org/r/34256/diff/4/?file=962202#file962202line29>
> >
> > nice tests.
Can I drop this issue :)?
> On May 19, 2015, 8:07 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, lines 32-90
> > <https://reviews.apache.org/r/34256/diff/4/?file=962201#file962201line32>
> >
> > Can you add comments to the code here? It is really hard to follow what's happening.
> >
> > Alternatively, can you simplify these using "strings" functions?
> >
> > For example, looking at http://linux.die.net/man/3/basename
> > ```
> > The functions dirname() and basename() break a null-terminated pathname string into directory and filename components. In the usual case, dirname() returns the string up to, but not including, the final '/', and basename() returns the component following the final '/'. Trailing '/' characters are not counted as part of the pathname.
> >
> > If path does not contain a slash, dirname() returns the string "." while basename() returns a copy of path. If path is the string "/", then both dirname() and basename() return the string "/". If path is a NULL pointer or points to an empty string, then both dirname() and basename() return the string ".".
> > ```
> >
> > this is how I would implement basename
> >
> > ```
> > string basename()
> > {
> > // Remove trailing "/", if exists.
> > string result = strings::remove(value, "/", strings::SUFFIX);
> >
> > if (result.empty()) {
> > return string(".");
> > }
> >
> > if (!strings::contains(result, "/")) {
> > return result;
> > }
> >
> > if (result == "/") {
> > return result;
> > }
> >
> > vector<string> tokens = strings::tokenize(result, "/");
> > return tokens[tokens.size() - 1];
> > }
> > ```
> >
> > I haven't checked all the edge cases, but you get the idea.
I thought a while about how we could use strings::remove but unfortunately I did not see a good way to make sure that e.g. multiple trailing slashes are properly cut off. In the end I decided that using an indexed based parsing would be more efficient and by the added comments also readable fine - at least that is what I think :)
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34256/#review84375
-----------------------------------------------------------
On May 17, 2015, 7:46 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34256/
> -----------------------------------------------------------
>
> (Updated May 17, 2015, 7:46 p.m.)
>
>
> Review request for mesos and Cody Maloney.
>
>
> Bugs: MESOS-1303
> https://issues.apache.org/jira/browse/MESOS-1303
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Introducing Path::dirname() and Path::basename() as a thread safe replacement of the respective system functions. Also contains new tests covering corner cases.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df650
> 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf35412
>
> Diff: https://reviews.apache.org/r/34256/diff/
>
>
> Testing
> -------
>
> make check (including new tests).
>
> Result comparison to match ::dirname and ::basename on interesting cases.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 34256: Added Path::dirname() and Path::basename().
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34256/#review84375
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/34256/#comment135590>
Can you add comments to the code here? It is really hard to follow what's happening.
Alternatively, can you simplify these using "strings" functions?
For example, looking at http://linux.die.net/man/3/basename
```
The functions dirname() and basename() break a null-terminated pathname string into directory and filename components. In the usual case, dirname() returns the string up to, but not including, the final '/', and basename() returns the component following the final '/'. Trailing '/' characters are not counted as part of the pathname.
If path does not contain a slash, dirname() returns the string "." while basename() returns a copy of path. If path is the string "/", then both dirname() and basename() return the string "/". If path is a NULL pointer or points to an empty string, then both dirname() and basename() return the string ".".
```
this is how I would implement basename
```
string basename()
{
// Remove trailing "/", if exists.
string result = strings::remove(value, "/", strings::SUFFIX);
if (result.empty()) {
return string(".");
}
if (!strings::contains(result, "/")) {
return result;
}
if (result == "/") {
return result;
}
vector<string> tokens = strings::tokenize(result, "/");
return tokens[tokens.size() - 1];
}
```
I haven't checked all the edge cases, but you get the idea.
3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp
<https://reviews.apache.org/r/34256/#comment135593>
nice tests.
- Vinod Kone
On May 17, 2015, 7:46 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34256/
> -----------------------------------------------------------
>
> (Updated May 17, 2015, 7:46 p.m.)
>
>
> Review request for mesos and Cody Maloney.
>
>
> Bugs: MESOS-1303
> https://issues.apache.org/jira/browse/MESOS-1303
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Introducing Path::dirname() and Path::basename() as a thread safe replacement of the respective system functions. Also contains new tests covering corner cases.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df650
> 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf35412
>
> Diff: https://reviews.apache.org/r/34256/diff/
>
>
> Testing
> -------
>
> make check (including new tests).
>
> Result comparison to match ::dirname and ::basename on interesting cases.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 34256: Added Path::dirname() and Path::basename().
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34256/
-----------------------------------------------------------
(Updated June 24, 2015, 2:44 a.m.)
Review request for mesos and Cody Maloney.
Changes
-------
Addressed comments.
Bugs: MESOS-1303
https://issues.apache.org/jira/browse/MESOS-1303
Repository: mesos-incubating
Description
-------
Introducing Path::dirname() and Path::basename() as a thread safe replacement of the respective system functions. Also contains new tests covering corner cases.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 59595c9d0372cb7fc8ec5acbab91ee298b673167
3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 7dd266421377e3f531c15cdf17377141f26c2715
Diff: https://reviews.apache.org/r/34256/diff/
Testing
-------
make check (including new tests).
Result comparison to match ::dirname and ::basename on interesting cases.
Thanks,
Till Toenshoff
Re: Review Request 34256: Added Path::dirname() and Path::basename().
Posted by Till Toenshoff <to...@me.com>.
> On June 23, 2015, 2:37 p.m., Kapil Arya wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp, lines 40-52
> > <https://reviews.apache.org/r/34256/diff/7/?file=990279#file990279line40>
> >
> > It probably applies to the Class itself, but should we also be testing for repeated middle slashes like "/a//b/c//d/"? Or is it beyond the scope of Path class?
IMHO whatever ::basename / ::dirname produce on such input, we should match -- hence I am adding your suggested patterns (while also trying them on the standard variants).
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34256/#review88979
-----------------------------------------------------------
On June 23, 2015, 8:26 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34256/
> -----------------------------------------------------------
>
> (Updated June 23, 2015, 8:26 a.m.)
>
>
> Review request for mesos and Cody Maloney.
>
>
> Bugs: MESOS-1303
> https://issues.apache.org/jira/browse/MESOS-1303
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Introducing Path::dirname() and Path::basename() as a thread safe replacement of the respective system functions. Also contains new tests covering corner cases.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 59595c9d0372cb7fc8ec5acbab91ee298b673167
> 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 7dd266421377e3f531c15cdf17377141f26c2715
>
> Diff: https://reviews.apache.org/r/34256/diff/
>
>
> Testing
> -------
>
> make check (including new tests).
>
> Result comparison to match ::dirname and ::basename on interesting cases.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 34256: Added Path::dirname() and Path::basename().
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34256/#review88979
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp (lines 40 - 52)
<https://reviews.apache.org/r/34256/#comment141577>
It probably applies to the Class itself, but should we also be testing for repeated middle slashes like "/a//b/c//d/"? Or is it beyond the scope of Path class?
- Kapil Arya
On June 23, 2015, 4:26 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34256/
> -----------------------------------------------------------
>
> (Updated June 23, 2015, 4:26 a.m.)
>
>
> Review request for mesos and Cody Maloney.
>
>
> Bugs: MESOS-1303
> https://issues.apache.org/jira/browse/MESOS-1303
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Introducing Path::dirname() and Path::basename() as a thread safe replacement of the respective system functions. Also contains new tests covering corner cases.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 59595c9d0372cb7fc8ec5acbab91ee298b673167
> 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 7dd266421377e3f531c15cdf17377141f26c2715
>
> Diff: https://reviews.apache.org/r/34256/diff/
>
>
> Testing
> -------
>
> make check (including new tests).
>
> Result comparison to match ::dirname and ::basename on interesting cases.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 34256: Added Path::dirname() and Path::basename().
Posted by Till Toenshoff <to...@me.com>.
> On June 23, 2015, 5:29 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 76
> > <https://reviews.apache.org/r/34256/diff/7/?file=990278#file990278line76>
> >
> > don't you want to do the same short circuit as in basename() for path containing only slashes?
In `basename` that short-circuit is needed. For `basename` consider `"///"` as input, the expected (= `::basename`) output would be `"/"` but without that clause the resulting output would be `""`. In `dirname` however that is not needed. Same example as before, `"///"` is expected to return `"/"` and that is exactly what we get. Feel free to reopen this issue in case I missed something.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34256/#review88998
-----------------------------------------------------------
On June 24, 2015, 2:44 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34256/
> -----------------------------------------------------------
>
> (Updated June 24, 2015, 2:44 a.m.)
>
>
> Review request for mesos and Cody Maloney.
>
>
> Bugs: MESOS-1303
> https://issues.apache.org/jira/browse/MESOS-1303
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Introducing Path::dirname() and Path::basename() as a thread safe replacement of the respective system functions. Also contains new tests covering corner cases.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 59595c9d0372cb7fc8ec5acbab91ee298b673167
> 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 7dd266421377e3f531c15cdf17377141f26c2715
>
> Diff: https://reviews.apache.org/r/34256/diff/
>
>
> Testing
> -------
>
> make check (including new tests).
>
> Result comparison to match ::dirname and ::basename on interesting cases.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 34256: Added Path::dirname() and Path::basename().
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34256/#review88998
-----------------------------------------------------------
Ship it!
much cleaner. thank you!
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp (line 41)
<https://reviews.apache.org/r/34256/#comment141587>
extraneous "/" in slashes.
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp (line 45)
<https://reviews.apache.org/r/34256/#comment141588>
ditto.
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp (line 73)
<https://reviews.apache.org/r/34256/#comment141589>
ditto.
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp (line 76)
<https://reviews.apache.org/r/34256/#comment141590>
don't you want to do the same short circuit as in basename() for path containing only slashes?
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp (line 81)
<https://reviews.apache.org/r/34256/#comment141591>
s/non trailing// ?
- Vinod Kone
On June 23, 2015, 8:26 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34256/
> -----------------------------------------------------------
>
> (Updated June 23, 2015, 8:26 a.m.)
>
>
> Review request for mesos and Cody Maloney.
>
>
> Bugs: MESOS-1303
> https://issues.apache.org/jira/browse/MESOS-1303
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Introducing Path::dirname() and Path::basename() as a thread safe replacement of the respective system functions. Also contains new tests covering corner cases.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 59595c9d0372cb7fc8ec5acbab91ee298b673167
> 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 7dd266421377e3f531c15cdf17377141f26c2715
>
> Diff: https://reviews.apache.org/r/34256/diff/
>
>
> Testing
> -------
>
> make check (including new tests).
>
> Result comparison to match ::dirname and ::basename on interesting cases.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 34256: Added Path::dirname() and Path::basename().
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34256/
-----------------------------------------------------------
(Updated June 23, 2015, 8:26 a.m.)
Review request for mesos and Cody Maloney.
Bugs: MESOS-1303
https://issues.apache.org/jira/browse/MESOS-1303
Repository: mesos-incubating
Description
-------
Introducing Path::dirname() and Path::basename() as a thread safe replacement of the respective system functions. Also contains new tests covering corner cases.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 59595c9d0372cb7fc8ec5acbab91ee298b673167
3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 7dd266421377e3f531c15cdf17377141f26c2715
Diff: https://reviews.apache.org/r/34256/diff/
Testing
-------
make check (including new tests).
Result comparison to match ::dirname and ::basename on interesting cases.
Thanks,
Till Toenshoff
Re: Review Request 34256: Added Path::dirname() and Path::basename().
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34256/
-----------------------------------------------------------
(Updated June 5, 2015, 1:31 p.m.)
Review request for mesos and Cody Maloney.
Changes
-------
Removed some include artefact.
Bugs: MESOS-1303
https://issues.apache.org/jira/browse/MESOS-1303
Repository: mesos-incubating
Description
-------
Introducing Path::dirname() and Path::basename() as a thread safe replacement of the respective system functions. Also contains new tests covering corner cases.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df650
3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf35412
Diff: https://reviews.apache.org/r/34256/diff/
Testing
-------
make check (including new tests).
Result comparison to match ::dirname and ::basename on interesting cases.
Thanks,
Till Toenshoff
Re: Review Request 34256: Added Path::dirname() and Path::basename().
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34256/
-----------------------------------------------------------
(Updated June 5, 2015, 1:03 p.m.)
Review request for mesos and Cody Maloney.
Changes
-------
Some simplification, added more comments and did a rebase.
Bugs: MESOS-1303
https://issues.apache.org/jira/browse/MESOS-1303
Repository: mesos-incubating
Description
-------
Introducing Path::dirname() and Path::basename() as a thread safe replacement of the respective system functions. Also contains new tests covering corner cases.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df650
3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf35412
Diff: https://reviews.apache.org/r/34256/diff/
Testing
-------
make check (including new tests).
Result comparison to match ::dirname and ::basename on interesting cases.
Thanks,
Till Toenshoff
Re: Review Request 34256: Added Path::dirname() and Path::basename().
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34256/#review84504
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/34256/#comment135821>
+1
- Alexander Rojas
On May 17, 2015, 9:46 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34256/
> -----------------------------------------------------------
>
> (Updated May 17, 2015, 9:46 p.m.)
>
>
> Review request for mesos and Cody Maloney.
>
>
> Bugs: MESOS-1303
> https://issues.apache.org/jira/browse/MESOS-1303
>
>
> Repository: mesos-incubating
>
>
> Description
> -------
>
> Introducing Path::dirname() and Path::basename() as a thread safe replacement of the respective system functions. Also contains new tests covering corner cases.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df650
> 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf35412
>
> Diff: https://reviews.apache.org/r/34256/diff/
>
>
> Testing
> -------
>
> make check (including new tests).
>
> Result comparison to match ::dirname and ::basename on interesting cases.
>
>
> Thanks,
>
> Till Toenshoff
>
>