> 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.
Heh, this is why more eyes are better. Go with Joe's, it's much better. - Andrew ----------------------------------------------------------- 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 > >
