mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Hindman" <b...@berkeley.edu>
Subject Re: Review Request 25789: Variadic strings join for c++11 and above
Date Fri, 19 Sep 2014 10:25:41 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message