> On Oct. 8, 2017, 3:27 a.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/include/stout/path.hpp > > Lines 28-35 (patched) > > <https://reviews.apache.org/r/60622/diff/2/?file=1847150#file1847150line28> > > > > General note on comment style: Mesos / stout / libprocess uses `//` > > over `/*`, and requires full sentences (so that first sentence should end > > in a period). > > > > Why is '"normalizes"' quoted? > > > > Furthermore, the '"\"' is confusing in the comment. It conversts '/' to > > '\', which in code is escaped `\`, but let's be clear that it's a single > > forward slash to a single backward slash. > > > > Also, it's not that: > > > > > The Windows > > * file system APIs don't work in a rational way > > > > It's that the Windows APIs explicitly do not support forward slash as a > > path separator with long paths (i.e. those prefixed with '\?\'). (This is > > documented behavior, not "irrational.")
Note that // vs / with * wasn't the case at line 271, which is why I commented the way I did. I figured internally consistent was better than internally inconsistent. But I went ahead and changed /* */ to //. As for "normalizes", that's a term that I felt I was inventing for this (thus the quotes). Removed. "Irrational" is debatable; I'd expect at least CONSISTENT behavior, not behavior that depends on the length of the filename. But I did rework what I said. > On Oct. 8, 2017, 3:27 a.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/include/stout/path.hpp > > Lines 38-41 (patched) > > <https://reviews.apache.org/r/60622/diff/2/?file=1847150#file1847150line38> > > > > This appears superfluous. Optimization for that case. But I removed it. > On Oct. 8, 2017, 3:27 a.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/include/stout/path.hpp > > Lines 45 (patched) > > <https://reviews.apache.org/r/60622/diff/2/?file=1847150#file1847150line45> > > > > This is probably fine for now, but what edges case might it miss? I don't understand what you're asking. Doing this conversion is the entire point of the function! Edge cases that I know of: None. On Windows, you can't have either "/" or "\" as part of a filename (they delimit directories). That's documented behavior. > On Oct. 8, 2017, 3:27 a.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/include/stout/path.hpp > > Lines 29-38 (original), 51-60 (patched) > > <https://reviews.apache.org/r/60622/diff/2/?file=1847150#file1847150line51> > > > > Why are we normalizing `path::join`? I thought we explicitly only > > needed to normalizes paths that are being converted into URIs. This was a safety thing. The class will normalize, but then if something is joined that isn't normalized, bad things happen (partically normalized does't count). This is documented. I'll go back and touch up the failure cases to make this unnecessary. My guess is that will require more code changes than this, but so be it. > On Oct. 8, 2017, 3:27 a.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/include/stout/uri.hpp > > Lines 33 (patched) > > <https://reviews.apache.org/r/60622/diff/2/?file=1847151#file1847151line33> > > > > Can this have a default `scheme = Scheme::FILE`? That would make it nicer. But I tend to like to be explicit to not create bugs (like the wrong scheme). Given that there's only one today, I'll make the change. > On Oct. 8, 2017, 3:27 a.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/include/stout/uri.hpp > > Lines 35-38 (patched) > > <https://reviews.apache.org/r/60622/diff/2/?file=1847151#file1847151line35> > > > > This seems superfluous, and what's more, breaks the expectation that > > all outputs of this function are prefixed with the scheme. > > > > Perhaps an empty string as an input is an error, but I think it's > > unexpected that it would be treated specially and just returned without > > further processing (or returning an error). This was more an error case; it seemed reasonable to return nothing if nothing was passed in. I'll take this as "legal" and just return "file://" in this case, I guess. > On Oct. 8, 2017, 3:27 a.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/include/stout/uri.hpp > > Lines 41-46 (patched) > > <https://reviews.apache.org/r/60622/diff/2/?file=1847151#file1847151line41> > > > > This `switch (enum scheme` feels convoluted (especially considerin > > there is only one currently supported scheme). It also makes it a hassle > > for the user to change schemes (as they have to modify this code). Why not > > take as an argument `string scheme = "file://"`? It would be reasonable to > > document that it expects `scheme + separator`. Yuck, taking a string is just opportunities for typos where the compiler could have caught it. Given the current state of the code, I'm going to change this to just not take a scheme at all. We'll do "file://" all the time and, if in the future we need to refactor for something more flexible, so be it. > On Oct. 8, 2017, 3:27 a.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/include/stout/uri.hpp > > Lines 48-52 (patched) > > <https://reviews.apache.org/r/60622/diff/2/?file=1847151#file1847151line48> > > > > Wait, so why did we add normalize above, when we then hand-rolled URI > > path normalization here? Removed include file. Note that normalize goes the other way (/ to ). This is returning a uri. > On Oct. 8, 2017, 3:27 a.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/include/stout/uri.hpp > > Lines 48-52 (patched) > > <https://reviews.apache.org/r/60622/diff/2/?file=1847151#file1847151line48> > > > > Pre-processor directives should always start at the beginning of the > > line > > (https://google.github.io/styleguide/cppguide.html#Preprocessor_Directives). Hey, I didn't know that. Google's style guide == Mesos style guide? > On Oct. 8, 2017, 3:27 a.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/tests/uri_tests.cpp > > Lines 19 (patched) > > <https://reviews.apache.org/r/60622/diff/2/?file=1847154#file1847154line19> > > > > Where was `string` ever used? Not anymore! - Jeff ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60622/#review187347 ----------------------------------------------------------- On Oct. 11, 2017, 11:31 p.m., Jeff Coffler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60622/ > ----------------------------------------------------------- > > (Updated Oct. 11, 2017, 11:31 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and > Li Li. > > > Bugs: MESOS-6705 > https://issues.apache.org/jira/browse/MESOS-6705 > > > Repository: mesos > > > Description > ------- > > Added new stout function: path::normalize (normalize a filename), > along with uri::from_path (generate URI from file path). > > > Diffs > ----- > > 3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 > 3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 > 3rdparty/stout/include/stout/path.hpp > 6ee3a44cd6a878fe383aa68df40b82857b93d0b4 > 3rdparty/stout/include/stout/uri.hpp PRE-CREATION > 3rdparty/stout/tests/CMakeLists.txt > 6e5773f1e03671de7ac007caf49edd0f1cd7aedd > 3rdparty/stout/tests/path_tests.cpp > f8c14d5aefe0b49adb778da784143a328c96183d > 3rdparty/stout/tests/uri_tests.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/60622/diff/3/ > > > Testing > ------- > > See upstream > > > Thanks, > > Jeff Coffler > >
