> 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. > > Jeff Coffler wrote: > 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.
I just pushed a new review, this one was not addressed pending feedback from Joe. > On April 25, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/include/stout/path.hpp > > Lines 79-86 (patched) > > <https://reviews.apache.org/r/58673/diff/2/?file=1699763#file1699763line79> > > > > I think this got a bit out of date. We settled on `\...` being an > > absolute path, not just `\?...` (per below implementation). > > Jeff Coffler wrote: > I didn't have a test for this specifically. The above is accurate, > though. The 1-3 cases are for files on disk. Network shares are a special > case and, while technically an absolute path, isn't related to files on disk. > > That said, the function does implement this properly. The comment isn't > out of date. The note was just to point out that we handled this as well. > > Andrew Schwartzmeyer wrote: > Sorry, I mean that your third bullet should read `3. "\..."` instead of > `3. "\?..."`. The latest review clarifies that these paths are for files on Windows. > 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 == ":/"; > ``` > > Jeff Coffler wrote: > 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. > > Andrew Schwartzmeyer wrote: > Heh, this is why more eyes are better. Go with Joe's, it's much better. Latest review implements Joe's suggestion (without lambda). Tests pass successfully. > On April 25, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote: > > 3rdparty/stout/tests/CMakeLists.txt > > Lines 44-59 (original), 44-59 (patched) > > <https://reviews.apache.org/r/58673/diff/2/?file=1699764#file1699764line44> > > > > Personally, I am very liberal with commits, and would have a separate > > patch to touch the build system. But if Joe's happy, I'm happy. > > Jeff Coffler wrote: > Six of one, a dozen of another. I viewed the unit test changes as "part > of" the implementation, so I put them in a single commit. If Joe prefers, I > can separate them out. I suppose I could have had one for the implementation, > and then one for the unit test changes (and cmake changes, they were related). > > In my mind, all code should be tested, so the unit tests were "part of" > the implementation change. Thus, one commit. But if you take the viewpoint of > "each commit should stand on it's own", I could have easily had a separate > commit for the cmake and unit test changes. > > Joe's call, if he wants me to make a revision, I'm happy to do it. > > Joseph Wu wrote: > Keep the build change together with this one. > > If you consider the change, you're basically adding a new file. And it > wouldn't make too much sense to create a new file _without_ making a build > file change. No change made as per Joe's comments. > 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. > > Jeff Coffler wrote: > 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. New tests added as per my above comments. New tests (as well as original tests) pass on Windows. Linux tests unaffected (but I did run them, they still pass). - Jeff ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58673/#review173000 ----------------------------------------------------------- On April 28, 2017, 9:43 p.m., Jeff Coffler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58673/ > ----------------------------------------------------------- > > (Updated April 28, 2017, 9:43 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/3/ > > > 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 > >
