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 Tue, 17 Apr 2018 14:35:43 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.
> 
> Benjamin Bannier wrote:
>     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?

@alexr: I updated the patch with an additional overload.

I first tried to consume a generic value type which I then could `std::forward`, but unfortunately
we put pointers to overloaded functions as values into `hashmap` which leads to problems deducing
the required type for `put`.


- Benjamin


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


On April 17, 2018, 4:35 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66608/
> -----------------------------------------------------------
> 
> (Updated April 17, 2018, 4:35 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 relaxes the requirements on the value argument to `put` so
> that instead of copyable we now only require move-constructible types.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/include/stout/hashmap.hpp 91085b8d8ad5d35c39c8cc95e3d4765d82d9a8db 
> 
> 
> Diff: https://reviews.apache.org/r/66608/diff/2/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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