----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25084/#review51743 -----------------------------------------------------------
Looks pretty good. Just a couple questions about #if logic, testing trailing slashes, and minor style nits. 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp <https://reviews.apache.org/r/25084/#comment90311> Shouldn't this #if logic be reversed, such that "strings.hpp" is included if < 2011? 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp <https://reviews.apache.org/r/25084/#comment90314> Style nit: We typically use "char* foo" over "char *foo" 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp <https://reviews.apache.org/r/25084/#comment90322> We weren't trimming trailing slashes before. Would this be a difference in behavior between the C++11 version and the old version? Did you run the unit tests both with & without C++11 enabled? 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp <https://reviews.apache.org/r/25084/#comment90315> What happens here if part==NULL? then end = NULL and we try to decrement it? Probably should assert/early-exit if (part==NULL) 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp <https://reviews.apache.org/r/25084/#comment90318> Style nit: Either end this comment with punctuation ('.') or remove it. 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp <https://reviews.apache.org/r/25084/#comment90323> Please add Apache license block. 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp <https://reviews.apache.org/r/25084/#comment90325> Two of these look identical. Am I blind, or did you mean for one to test chars with join('a', 'b', 'c')? 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp <https://reviews.apache.org/r/25084/#comment90326> Do these trailing slash tests pass without your new C++11 changes? Does the old code need to be updated? - Adam B On Aug. 27, 2014, 5:51 p.m., Patrick Reilly wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25084/ > ----------------------------------------------------------- > > (Updated Aug. 27, 2014, 5:51 p.m.) > > > Review request for mesos, Adam B and Benjamin Hindman. > > > Bugs: MESOS-1733 > https://issues.apache.org/jira/browse/MESOS-1733 > > > Repository: mesos-git > > > Description > ------- > > Changed the stout path utility to declare a single, variadic 'join' function > instead of several separate declarations of various discrete arities > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/Makefile.am db9766d > 3rdparty/libprocess/3rdparty/stout/Makefile.am b6464de > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp bc6920a > 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/25084/diff/ > > > Testing > ------- > > Ran "make check" created path_tests unit test. > > > Thanks, > > Patrick Reilly > >
