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