mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Bannier" <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 41875: Removed List type which is not needed anymore with C++11.
Date Mon, 11 Jan 2016 13:14:51 GMT


> On Jan. 11, 2016, 1:34 p.m., Alexander Rojas wrote:
> > As a patch it looks good, however I feel the reason we created these types inheriting
from the STL containers was to add a functionality to them (like in `hashmap` and `hashset`).
But I feel this class doesn't add anything to the default `std::list<T>` and indeed
it removes functionality since now there's no access to all the different constructors from
`std::list` and we run in the risk of leaking memory (One should not create pointers to any,
`List`, `hashmap` or `hashset`).
> > 
> > My preferred solution would be to delete this convenience class, and use a typedef
while the code that uses it is cleaned up, or just remove it altogether and cleanup in one
pass.

Excellent point, and looking back at the original rr this is what was intended.


- Benjamin


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


On Jan. 11, 2016, 2:14 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41875/
> -----------------------------------------------------------
> 
> (Updated Jan. 11, 2016, 2:14 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Till Toenshoff.
> 
> 
> Bugs: MESOS-4273
>     https://issues.apache.org/jira/browse/MESOS-4273
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This type was added to provide a variadic constructor around a
> std::list. Now that C++11 is used this is not needed anymore as we can
> directly invoke std::list's constructor taking a std::initializer_list.
> 
> Review: https://reviews.apache.org/r/41875
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe

>   3rdparty/libprocess/3rdparty/stout/include/stout/list.hpp 864d8c9498d66f14ab6fc531965c12fb397b5fe5

> 
> Diff: https://reviews.apache.org/r/41875/diff/
> 
> 
> Testing
> -------
> 
> make check (OS X 10.10.5 and Debian 8)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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