> 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).
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. > 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. 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! :-) > 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. 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. > 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`. 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. - 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 > >
