> On April 25, 2017, 4: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`.
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. > On April 25, 2017, 4: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! :-) 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 == ":/"; ``` > On April 25, 2017, 4: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. 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. > On April 25, 2017, 4: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. The more (comprehensive) the merrier :) For example, this could use some more drive letter name variety. - Joseph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58673/#review173000 ----------------------------------------------------------- On April 25, 2017, 11:24 a.m., Jeff Coffler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58673/ > ----------------------------------------------------------- > > (Updated April 25, 2017, 11:24 a.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 > >
