Re: Review Request 25789: Variadic strings join for c++11 and above
--- 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
Re: Review Request 25789: Variadic strings join for c++11 and above
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25789/ --- (Updated Sept. 19, 2014, 6:37 p.m.) Review request for mesos and Benjamin Hindman. Changes --- Add overloads of an internal::append function to support mixed type strings::join(). There are special overloads for string ands const char * to maintain performance. Added a test for mixed type strings::join(). Fixed style issues. 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 (updated) - 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
Re: Review Request 25789: Variadic strings join for c++11 and above
On Sept. 19, 2014, 10:25 a.m., Benjamin Hindman wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp, line 205 https://reviews.apache.org/r/25789/diff/4/?file=694225#file694225line205 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! I made the variadic strings::join() more generic in that it now uses a helper function called append(). This in the general case falls back to stringify, but binds first to special versions for string and const char * that have low overhead. - Joris --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25789/#review53949 --- On Sept. 19, 2014, 6:37 p.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, 6:37 p.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
Re: Review Request 25789: Variadic strings join for c++11 and above
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25789/#review53862 --- 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp https://reviews.apache.org/r/25789/#comment93725 given that we test for variadic template support in configure, do you think we still need this? - Dominic Hamon On Sept. 18, 2014, 12:58 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25789/ --- (Updated Sept. 18, 2014, 12:58 p.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
Re: Review Request 25789: Variadic strings join for c++11 and above
On Sept. 18, 2014, 8:11 p.m., Dominic Hamon wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp, line 190 https://reviews.apache.org/r/25789/diff/1/?file=693848#file693848line190 given that we test for variadic template support in configure, do you think we still need this? I'm on board with not checking this anymore as long as everyone is on board with that. - Joris --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25789/#review53862 --- On Sept. 18, 2014, 7:58 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25789/ --- (Updated Sept. 18, 2014, 7:58 p.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
Re: Review Request 25789: Variadic strings join for c++11 and above
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25789/ --- (Updated Sept. 18, 2014, 10:22 p.m.) Review request for mesos and Benjamin Hindman. Changes --- Deal with a prototype collision between the Iterable Join and variadic join by requiring multiple arguments. Refactor the helper class pattern to basic function recursion within a helper namespace. 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 (updated) - 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
Re: Review Request 25789: Variadic strings join for c++11 and above
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25789/#review53891 --- It seems we have mixed use of const char * and std::string in the usage of join. Dealing with this and writing test for this now. - Joris Van Remoortere On Sept. 18, 2014, 10:22 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25789/ --- (Updated Sept. 18, 2014, 10:22 p.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
Re: Review Request 25789: Variadic strings join for c++11 and above
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25789/ --- (Updated Sept. 18, 2014, 11:10 p.m.) Review request for mesos and Benjamin Hindman. Changes --- Dealt with mixed use between (const char *) and (std::string) in a single join() call. Accompanying unit test. 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 (updated) - 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
Re: Review Request 25789: Variadic strings join for c++11 and above
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25789/#review53898 --- 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp https://reviews.apache.org/r/25789/#comment93764 We don't need an inline here any longer as template implies that the function is inline. (Same goes for the rest of these) - Cody Maloney On Sept. 18, 2014, 11:10 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25789/ --- (Updated Sept. 18, 2014, 11:10 p.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
Re: Review Request 25789: Variadic strings join for c++11 and above
--- 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. Changes --- remove explicit inlines. Fix style issues. 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 (updated) - 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
