----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25789/#review53949 -----------------------------------------------------------
Mostly just style stuff, after a quick cleanup we'll get this committed. Thanks Joris! 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp <https://reviews.apache.org/r/25789/#comment93826> Just a minor thing getting use to the code base, we've used 'internal' instead of 'helper' in public header files for things we don't want to expose. 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp <https://reviews.apache.org/r/25789/#comment93828> Another style aspect in our codebase, we just use 'T' rather than the more verbose 'TVal'. Or we'll vary the single letter if it matches the kind of argument well, in this case, just 'T' is sufficient. 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp <https://reviews.apache.org/r/25789/#comment93827> Style wise, please move 'std::stringstream&' to a newline, here and everywhere else please. 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp <https://reviews.apache.org/r/25789/#comment93829> At first I was expecting strings::join to just be variadic on std::string (like the original strings::join functions). We have a 'stringify' operation that we use which ultimately just uses operator <<, will we get the same result with the std::string() conversion as we do with stringify? Irregardless, let's make sure that we have tests which are joining more than just strings if we expect to get more than just strings, and that the semantics are expected with stringify! 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp <https://reviews.apache.org/r/25789/#comment93830> Maybe for consistency use 'tail' here instead of 'rest'? Also, style wise, please move &&.. to type as elsewhere. 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp <https://reviews.apache.org/r/25789/#comment93831> Stylewise, we put each argument on their own line, thanks! - Benjamin Hindman On Sept. 19, 2014, 12:36 a.m., Joris Van Remoortere wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25789/ > ----------------------------------------------------------- > > (Updated Sept. 19, 2014, 12:36 a.m.) > > > Review request for mesos and Benjamin Hindman. > > > Repository: mesos-git > > > Description > ------- > > Add Variadic strings join for c++11 and above. > There is a second version of the variadic join which takes a reference to a > stringstream as a parameter. This is handy when strings::join is just a part > of a larger string manipulation. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp a1702cd > 3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 51008e5 > > Diff: https://reviews.apache.org/r/25789/diff/ > > > Testing > ------- > > Ran make check for stout. Added test cases for join as these were missing. > > > Thanks, > > Joris Van Remoortere > >
