> On April 25, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/flags/parse.hpp
> > Line 96 (original), 96 (patched)
> > <https://reviews.apache.org/r/58673/diff/2/?file=1699762#file1699762line96>
> >
> > There are more instances of erroneous absolute path checking through
> > the code base, some of which we should probably fix.
> >
> > That said, this change should probably be in a separate commit/patch to
> > address the particular bug, followed by more patches for the other
> > instances (which don't need to come immediately). But I don't think I'd
> > include this directly with the porting of `path::absolute`.
>
> Joseph Wu wrote:
> This particular chunk of code is something long deprecated, so even if
> the "correct" thing is to use `path::absolute`, we shouldn't change it.
>
> Instead (in a separate patch, and only if you want to), we should simply
> `#ifndef __WINDOWS__` the entire conditional and the body of the if-statement.
I don't quite understand this, as the end result is identical (on non-Windows
platforms). The old line here, as you can see, is:
```
if (strings::startsWith(value, "/")) {
```
This change calls `path::absolute`. That routine, for Linux, is doing the
identical thing:
```
#ifndef __WINDOWS__
return strings::startsWith(path, os::PATH_SEPARATOR);
```
It feels like changing the code here to `#ifndef __WINDOWS__` is adding
changes/complexity when the end result is exactly what you're asking for (no
behaviorial change).
Is your concern here that, for Linux, we may change what we believe an
"absolute path" to be? While possible, that strikes me as exceedingly unlikely.
For a VERY long time, the way to tell (on Linux) that a path is absolute is to
just check the first byte of the string.
Please clarify what your concern is here. I'd like to understand what you're
thinkin' here, thanks.
> On April 25, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 92-108 (patched)
> > <https://reviews.apache.org/r/58673/diff/2/?file=1699763#file1699763line92>
> >
> > I'm hoping Joe approves of this style; it's what I came up with while
> > working with Jeff. It seems to be the most readable.
> >
> > For what it's worth, we also tried `std::regex`, and profiled it. Even
> > in release mode with explicit optimization, it was 70x slower.
>
> Jeff Coffler wrote:
> Yeah. Before this, the code was essentially the same (a bunch of boolean
> conditionals), but rather "ugly" (three or four lines to represent the
> conditions with `||` and `&&`). At first I resisted Andy's suggestion, but
> ultimately decided it was a lot more readable, so I adopted it.
>
> Andy strongly recommended using a RegEx. While use of a RegEx allows for
> a nice one-liner on Windows, it was problematic:
>
> 1. Windows and Linux behaved differently. Indeed, Linux would fail given
> a Regex that was perfectly valid and worked fine on Windows. Ultimately, this
> didn't matter, though, since the Windows solution was Windows-specific.
>
> 2. I've worked with RegEx engines before, and I knew that it wouldn't
> perform. RegEx engines are great for a lot of cases, but this one (a few
> hard-coded boolean comparisons) isn't one of those cases. And indeed, a RegEx
> for this was > 70x slower, which is pretty dramatic. This code is ultimately
> a few boolean conditions, although it's 15 lines of C++ code. It is nice and
> readable, though, if you understand lambda expressions.
>
> I liked this in the end, so this was what I submitted. But behind it was
> performance analysis and all! :-)
>
> Joseph Wu wrote:
> Hm... I'm not seeing why the lambda is necessary here...
>
> Don't we get the same result by putting all four boolean conditions in
> sequence?
>
> ```
> if (strings::startsWith(path, "\\")) {
> return true;
> }
>
> if (path.length() < 3) {
> return false;
> }
>
> std::string letter = path.substr(0, 1);
> if (!((letter >= "A" && letter <= "Z") ||
> (letter >= "a" && letter <= "z"))) {
> return false;
> }
>
> std::string colon = path.substr(1, 2);
> return colon == ":\" || colon == ":/";
> ```
We could do that, and that's plenty readable. The original code did the same
sort of thing, but in a more compressed way. I don't have the original, but it
was along the line of:
```
return strings::startsWith(path, "\\")
|| ((path.length() >= 3 && (path.substr(1, 2) == ":\" || path.substr(1, 2) ==
":/")
&& ((path.substr(0, 1) >= "A" && path.substr(0, 1) <= "Z")
|| (path.substr(0, 1) >= "a" && path.substr(0, 1) <= "b")))
```
Something like that, anyway. I thought that the lambda was much better than
that. Your suggestion is fine, too, and is clear without the lambda. I'll
implement that and post another patch.
> On April 25, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/tests/path_tests.cpp
> > Lines 156-173 (patched)
> > <https://reviews.apache.org/r/58673/diff/2/?file=1699766#file1699766line156>
> >
> > I am happy to have these, although we're missing cases like a valid
> > network path `\server\share`, and our explicitly unsupported case of a
> > relative `\path`.
>
> Jeff Coffler wrote:
> Trivial to add new test cases now (just one line per test case). I can
> add whatever you'd like if Joe wants me to.
>
> Joseph Wu wrote:
> The more (comprehensive) the merrier :)
>
> For example, this could use some more drive letter name variety.
I'm going to do a change anyway to eliminate the lambda. I'll add a few tests
here, specifically for network paths, our explicitly unsupported case of
`\path` (since while Windows calls that "absolute", it's not - it depends on
the current disk of your CWD) and some additional drive letter names.
This will force a re-test on both Linux and Windows, so it'll take me a bit. I
hope to have a new review posted in the early afternoon.
- Jeff
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58673/#review173000
-----------------------------------------------------------
On April 25, 2017, 6:24 p.m., Jeff Coffler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58673/
> -----------------------------------------------------------
>
> (Updated April 25, 2017, 6:24 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and
> Li Li.
>
>
> Bugs: MESOS-5937
> https://issues.apache.org/jira/browse/MESOS-5937
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added 3rdparty/stout/tests/path_tests.cpp tests to Windows platform,
> but disabled tests that did not pass. Enabled absolute path tests,
> adding tests that were appropriate for the Windows platform.
>
> Note that, for Windows, absolute paths may not be valid. For example,
> a path like "\\?\abc:file.txt" is not valid. In this case, the
> path::absolute method is undefined; the expectation is that it is
> called with valid paths (absolute or relative, but valid).
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/flags/parse.hpp
> 65edd86372596c2107e9f29cf27301e025e6620e
> 3rdparty/stout/include/stout/path.hpp
> 2d2088aadfa1ea82c59424242671c4fb655dede1
> 3rdparty/stout/tests/CMakeLists.txt
> 4bbe713f259e7858d423dcb33956d41e62a915eb
> 3rdparty/stout/tests/flags_tests.cpp
> e2681f8f68f6478d4c8a20c1e75ddb050d16b56d
> 3rdparty/stout/tests/path_tests.cpp
> 0490d93908566c46a10d91b05790e5a7f2f289bc
>
>
> Diff: https://reviews.apache.org/r/58673/diff/2/
>
>
> Testing
> -------
>
> Passes `make check` on the Linux platform.
>
> On Windows, the FlagsFileTest.JSONFile now passes, along with new
> PathTest.Absolute tests that I added due to new path::absolute handling on
> the Windows platform.
>
> PS C:\mesos> .\build\3rdparty\stout\tests\Debug\stout-tests.exe
> --gtest_filter="*FlagsFileTest*"
> Note: Google Test filter = *FlagsFileTest*-
> [==========] Running 2 tests from 1 test case.
> [----------] Global test environment set-up.
> [----------] 2 tests from FlagsFileTest
> [ RUN ] FlagsFileTest.JSONFile
> WARNING: Logging before InitGoogleLogging() is written to STDERR
> W0425 10:05:20.959357 1060440 parse.hpp:97] Specifying an absolute filename
> to read a command line option out of without using 'file:// is deprecated and
> will be removed in a future release. Simply adding 'file://' to the beginning
> of the path should eliminate this warning.
> [ OK ] FlagsFileTest.JSONFile (9 ms)
> [ RUN ] FlagsFileTest.FilePrefix
> [ OK ] FlagsFileTest.FilePrefix (6 ms)
> [----------] 2 tests from FlagsFileTest (18 ms total)
>
> [----------] Global test environment tear-down
> [==========] 2 tests from 1 test case ran. (23 ms total)
> [ PASSED ] 2 tests.
> PS C:\mesos> .\build\3rdparty\stout\tests\Debug\stout-tests.exe
> --gtest_filter="*PathTest*"
> Note: Google Test filter = *PathTest*-
> [==========] Running 2 tests from 1 test case.
> [----------] Global test environment set-up.
> [----------] 2 tests from PathTest
> [ RUN ] PathTest.Absolute
> [ OK ] PathTest.Absolute (1 ms)
> [ RUN ] PathTest.Comparison
> [ OK ] PathTest.Comparison (0 ms)
> [----------] 2 tests from PathTest (2 ms total)
>
> [----------] Global test environment tear-down
> [==========] 2 tests from 1 test case ran. (6 ms total)
> [ PASSED ] 2 tests.
>
> YOU HAVE 4 DISABLED TESTS
>
> PS C:\mesos>
>
>
> Thanks,
>
> Jeff Coffler
>
>
