Re: Review Request 25789: Variadic strings join for c++11 and above

[Joris Van Remoortere]

> On Sept. 19, 2014, 10:25 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp, line 205
> > 
> >
> > 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

[Joris Van Remoortere]

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

[Benjamin Hindman]

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


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


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


Style wise, please move 'std::stringstream&' to a newline, here and 
everywhere else please.



3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp


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


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


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

[Joris Van Remoortere]

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

Re: Review Request 25789: Variadic strings join for c++11 and above

[Cody Maloney]

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


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

[Joris Van Remoortere]

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

[Joris Van Remoortere]

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

[Joris Van Remoortere]

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

[Joris Van Remoortere]

> On Sept. 18, 2014, 8:11 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp, line 190
> > 
> >
> > 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

[Dominic Hamon]

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


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

Review Request 25789: Variadic strings join for c++11 and above

[Joris Van Remoortere]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25789/
---

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