Re: Review Request 34256: Added Path::dirname() and Path::basename().

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