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