----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60622/#review187347 -----------------------------------------------------------
I didn't see _why_ we needed to add `path::normalize` and then only use it in `path::join` (and then only on the result, not on the inputs). The URI logic is the only logic that needs to worry about normalizing paths currently, simply because of the sepcification of a URI. 3rdparty/stout/include/stout/path.hpp Lines 28-35 (patched) <https://reviews.apache.org/r/60622/#comment264293> 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.") 3rdparty/stout/include/stout/path.hpp Lines 38-41 (patched) <https://reviews.apache.org/r/60622/#comment264294> This appears superfluous. 3rdparty/stout/include/stout/path.hpp Lines 45 (patched) <https://reviews.apache.org/r/60622/#comment264295> This is probably fine for now, but what edges case might it miss? 3rdparty/stout/include/stout/path.hpp Lines 29-38 (original), 51-60 (patched) <https://reviews.apache.org/r/60622/#comment264296> Why are we normalizing `path::join`? I thought we explicitly only needed to normalizes paths that are being converted into URIs. 3rdparty/stout/include/stout/uri.hpp Lines 30-32 (patched) <https://reviews.apache.org/r/60622/#comment264299> Ditto on comment style (I'll stop mentioning this and let the issue cover the rest of the review). 3rdparty/stout/include/stout/uri.hpp Lines 33 (patched) <https://reviews.apache.org/r/60622/#comment264297> Can this have a default `scheme = Scheme::FILE`? 3rdparty/stout/include/stout/uri.hpp Lines 35-38 (patched) <https://reviews.apache.org/r/60622/#comment264298> 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). 3rdparty/stout/include/stout/uri.hpp Lines 41-46 (patched) <https://reviews.apache.org/r/60622/#comment264300> 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`. 3rdparty/stout/include/stout/uri.hpp Lines 48-52 (patched) <https://reviews.apache.org/r/60622/#comment264301> Wait, so why did we add normalize above, when we then hand-rolled URI path normalization here? 3rdparty/stout/include/stout/uri.hpp Lines 48-52 (patched) <https://reviews.apache.org/r/60622/#comment264303> Pre-processor directives should always start at the beginning of the line (https://google.github.io/styleguide/cppguide.html#Preprocessor_Directives). 3rdparty/stout/include/stout/uri.hpp Lines 52 (patched) <https://reviews.apache.org/r/60622/#comment264302> This needs to be `#endif // __WINDOWS__`. 3rdparty/stout/tests/path_tests.cpp Lines 223-232 (patched) <https://reviews.apache.org/r/60622/#comment264305> If these got more complicated, you could use [Value Parameterized Tests](https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#value-parameterized-tests) (or even just iterate over pairs). 3rdparty/stout/tests/uri_tests.cpp Lines 19 (patched) <https://reviews.apache.org/r/60622/#comment264306> Where was `string` ever used? - Andrew Schwartzmeyer On Oct. 5, 2017, 9:03 p.m., Jeff Coffler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60622/ > ----------------------------------------------------------- > > (Updated Oct. 5, 2017, 9:03 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 > ------- > > Add new stout function: path::normalize (normalize a filename). > Add new stout function: uri::uriFromFilename (generate URI from > filename). > > > 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/2/ > > > Testing > ------- > > See upstream > > > Thanks, > > Jeff Coffler > >
