> On Oct. 17, 2014, 9:01 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 33 > > <https://reviews.apache.org/r/26766/diff/1/?file=722431#file722431line33> > > > > Hm.. what is asAbsolute doing? I would have expected this to be > > equivalent to `realpath()` from the signature name. That is, convert a > > relative path to an absolute path. However, this just prepends a slash? > > Seems like a fairly arbitrary path manipulation?
In all path manipulation code I've written going from a relative path to an absolute path is fairly common. I could make it so people could specify a prefix (So it effectively operates like --prefix in './configure') if that would make it make more sense as a helper function. Currently it is hard-coded that the prefix is '/'. In terms of path specification, we always expect absolute paths which are rooted at '/'. In some of the tests we specify relative paths which don't begin with '/'. In some tests of a subset of the files API we use '/' prefixed paths. From the Mesos WebUI we always use absolute paths. This function makes it so I can handle all those cases without really thinking about it. A name like 'ensureAbsolute' might be more relevant. For now I changed the name to 'prefix', because that is a filesystem path manipulation concept people are more familiar with. > On Oct. 17, 2014, 9:01 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 48 > > <https://reviews.apache.org/r/26766/diff/1/?file=722431#file722431line48> > > > > This confuses me since it appears to be an identity function at first > > glance. > > > > For comparision, imagine if I was reading the following very similar > > identity function: > > > > ``` > > // Returns exactly what you pass in. > > string identity(s) > > { > > return strings::join(",", strings::split(",", s)); > > } > > ``` > > > > Do we have the right abstractions here? Added a comment to note what this does. path::join() performs a certain level of argument cleaning in it's implementation. The main operation that is happening here, is removing redundant slashes. Join must remove redundant slashes as part of it's regular operation to behave properly The join() function behavior (Specifically the variadic one), is modeled after: https://docs.python.org/2/library/os.path.html#os.path.join For path stuff join and split generally don't round trip, and making them do so presents a lot of unnecessary complexity to their callers. Rather than just the special empty path at the beginning to differentiate join / split, I have to keep all empty paths in the middle of strings (Problematic when files does it's browse), and that there is a trailing slash or not has to be indicated and dealt with as well. - Cody ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26766/#review57213 ----------------------------------------------------------- On Oct. 23, 2014, 4:52 p.m., Cody Maloney wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26766/ > ----------------------------------------------------------- > > (Updated Oct. 23, 2014, 4:52 p.m.) > > > Review request for mesos, Ben Mahler and Timothy Chen. > > > Bugs: MESOS-1878 > https://issues.apache.org/jira/browse/MESOS-1878 > > > Repository: mesos-git > > > Description > ------- > > Adds 3 new functions: asAbsolute, clean, and split(). All three were > hand-coded inside of mesos files (files/files.cpp). This puts them in a > common place, and adds unit tests for their behavior. > > The functions depend on eachother somewhat, so I pulled out the declarations > to make them all forward declared. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp > 357a75a8bac497465671456aa9cd9181123cc635 > 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp > aedf93573ea89e46bf7b7b91f2258049af2fd79f > > Diff: https://reviews.apache.org/r/26766/diff/ > > > Testing > ------- > > make distcheck > > > Thanks, > > Cody Maloney > >
