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 66608: Improved support for move-only types in `hashmap`.
Date Mon, 16 Apr 2018 17:00:18 GMT


> On April 16, 2018, 6:56 p.m., Alexander Rukletsov wrote:
> > 3rdparty/stout/include/stout/hashmap.hpp
> > Line 104 (original), 104 (patched)
> > <https://reviews.apache.org/r/66608/diff/1/?file=1998223#file1998223line104>
> >
> >     Reading https://stackoverflow.com/questions/21035417/is-the-pass-by-value-and-then-move-construct-a-bad-idiom,
> >     
> >     I'm not sure what types are usually used as `Value`s in hashmaps. Are they expensive
to move? Are they expensive to copy? Can we say that you suggestion is a strict improvement?
Or at least a reasonable trade-off? I'm asking because I don't have neither enough experience
nor data to make a decision.

That's a valid concern. I consciously went for the simpler implementation here, but this being
in stout we are in pretty generic territory and it might make sense to go for the slightly
less naïve solution.

What's your feeling @mpark?


- Benjamin


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


On April 13, 2018, 3:09 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66608/
> -----------------------------------------------------------
> 
> (Updated April 13, 2018, 3:09 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> While it was already possible to create a `hashmap` over move-only
> values, we still performed a copy in `put`, making it hard to
> dynamically add elements with the expected stout semantics.
> 
> This patch releases the requirements on the value argument to `put` so
> that instead of copyable we now only require move-constructible.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/include/stout/hashmap.hpp 91085b8d8ad5d35c39c8cc95e3d4765d82d9a8db 
> 
> 
> Diff: https://reviews.apache.org/r/66608/diff/1/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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