mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Park" <mcyp...@gmail.com>
Subject Re: Review Request 37714: Updated Multimap and multihashmap so their signatures resemble that of hashmap and hashset.
Date Fri, 18 Sep 2015 23:48:55 GMT


> On Sept. 9, 2015, 11:46 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp, lines 96-97
> > <https://reviews.apache.org/r/37714/diff/3/?file=1057712#file1057712line96>
> >
> >     `s/std::make_pair(key, value)/{key, value}/`
> 
> Alexander Rojas wrote:
>     I wasn't sure if we are allowing initializer lists constructors. Some people have
complained about them in my reviews. Can you clarify? I for myself am all in.

I'm not sure, let's keep `std::make_pair` in case people have complaints.


> On Sept. 9, 2015, 11:46 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp, lines 107-117
> > <https://reviews.apache.org/r/37714/diff/3/?file=1057712#file1057712line107>
> >
> >     Can we just `auto` all of this away?
> >     
> >     ```
> >     auto range =
> >       std::unordered_multimap<Key, Value, Hash, Equal>::equal_range(key);
> >     for (auto it = range.first; it != range.second; ++it) {
> >       values.push_back(it->second);
> >     }
> >     ```
> 
> Alexander Rojas wrote:
>     Same here, I have gotten complains about using auto. Are we allowed to use it, if
so in which cases?

We have a relatively clear guideline for the use of `auto`. The following example is mentioned
as a valid use case:
```cpp
// 1: OK.
const auto i = values.find(keys.front());
// Compare with
const typename map::iterator i = values.find(keys.front());
```
Since `equal_range` is a standard library function like `find`, I think it's perfectly fine
to use `auto` here.


> On Sept. 9, 2015, 11:46 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/multimap.hpp, lines 34-37
> > <https://reviews.apache.org/r/37714/diff/3/?file=1057713#file1057713line34>
> >
> >     Can I ask why we want these? Presumably the purpose of these classes are to
eliminate the need for their `std::` counterparts.
> >     
> >     If we look at a class like `Set`, it inherits from `std::set` but doesn't construct
off of `std::set`. Similarly, `hashmap` inherits from `std::unordered_map`, but doesn't construct
off of `std::unordered_map`.
> 
> Alexander Rojas wrote:
>     I have no strong feelings about removing them. But if you check `hashset` it has
a constructor from a `set` just like this one, so I am just trying to keep up with their signatures.
I did find weird to have a constructor from `set` to `hashset` though. I'll drop this and
if you think I really ought to remove it, please reopen.

__TL;DR:__ The mapping from `std::set` -> `hashset` is different than `std::multimap` ->
`multimap`. Let's remove it.
> But if you check hashset it has a constructor from a set just like this one

I don't think they're quite the same. `hashset` and others construct off of a similar but
different data structure, not from the thing that they're intended to be replacing. As in,
since `hashset` is intended to replace `std::unordered_set`, it doesn't construct off `std::unordered_set`.
This `multimap` is intended to replace `std::multimap`, so it shouldn't construct off of `std::multimap`,
but it __could__ construct off of other similar classes such as `std::map`, for example.
> I did find weird to have a constructor from set to hashset though

Yep, that's because this pattern of constructing off of other similar classes runs into the
`N^2` combinatorial problem. As in, why shouldn't `hashset` construct off of `std::vector`,
`std::list`, `std::deque`, `std::array`, etc? This is why iterators were introduced into the
standard library in the first place, in order to decouple them. We could use templates and
contrain it such that we only accept containers that can be iterated on, but there's a lot
of work going into a v2 of the standard library with the emergence of concepts into the language.


- Michael


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


On Sept. 16, 2015, 6:14 a.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37714/
> -----------------------------------------------------------
> 
> (Updated Sept. 16, 2015, 6:14 a.m.)
> 
> 
> Review request for mesos, Joerg Schad, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-2924
>     https://issues.apache.org/jira/browse/MESOS-2924
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Adds extra template parameters to `multihashmap` which offer control over the hash function
to use as well as the equality operator.
> 
> Implements initializer_list, copy and move constructors for both, `multihashmap` and
`Multimap` in a similar way as it was done for `hashmap` and `hashset`.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp d9e4031cee64e48ad50541c04ca11e7861d0a17c

>   3rdparty/libprocess/3rdparty/stout/include/stout/multimap.hpp fb3e7a1d0377001389980302342813217f49cf5f

>   3rdparty/libprocess/3rdparty/stout/tests/multimap_tests.cpp 535cd2d10e3074c86c149ce85b205e73ca42ddd3

> 
> Diff: https://reviews.apache.org/r/37714/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


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