----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13464/#review25049 -----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/linkedhashmap.hpp <https://reviews.apache.org/r/13464/#comment49207> #include <utility> 3rdparty/libprocess/3rdparty/stout/include/stout/linkedhashmap.hpp <https://reviews.apache.org/r/13464/#comment49206> #include <stout/option.hpp> 3rdparty/libprocess/3rdparty/stout/include/stout/linkedhashmap.hpp <https://reviews.apache.org/r/13464/#comment49205> s/hashmap/HashMap/ then? ;) Eventually we can rename stout's data structures to use CamelCase. 3rdparty/libprocess/3rdparty/stout/include/stout/linkedhashmap.hpp <https://reviews.apache.org/r/13464/#comment49209> The advantage would be that LinkedHashMap could be used as a drop-in replacement for hashmap or map, since it would conform to the map interface. Extending from hashmap would require overriding the element access and modifier functions: [], insert, erase, swap, clear However, you would not need to implement many access operations: empty, size, max_size, find, count, lower_bound, upper_bound, equal_range, get_allocator, key_comp, value_comp The one scary bit would be that this will break when C++11 support is added so we would possibly want to #ifdef in the C++11 map functions. At the very least, can you implement the iteration operations since they were the whole point of adding this? begin(), end(), rbegin(), rend() These would let us do foreachvalue and foreachkey, which would be awesome! No copying of values needed :) 3rdparty/libprocess/3rdparty/stout/include/stout/linkedhashmap.hpp <https://reviews.apache.org/r/13464/#comment49204> Can you move these typedefs down into the private block where they are used? 3rdparty/libprocess/3rdparty/stout/include/stout/linkedhashmap.hpp <https://reviews.apache.org/r/13464/#comment49210> { on new line, ditto for the rest below 3rdparty/libprocess/3rdparty/stout/include/stout/linkedhashmap.hpp <https://reviews.apache.org/r/13464/#comment49212> Why does LinkedHashMap contain put(), whereas hashmap does not contain put()? What does put provide vs the [] operator? map.put(key, value) map[key] = value 3rdparty/libprocess/3rdparty/stout/include/stout/linkedhashmap.hpp <https://reviews.apache.org/r/13464/#comment49213> const 3rdparty/libprocess/3rdparty/stout/include/stout/linkedhashmap.hpp <https://reviews.apache.org/r/13464/#comment49214> s/void/size_t to conform to std::map interface http://www.cplusplus.com/reference/map/map/erase/ 3rdparty/libprocess/3rdparty/stout/include/stout/linkedhashmap.hpp <https://reviews.apache.org/r/13464/#comment49215> return values_.erase(key) here and return 0 below 3rdparty/libprocess/3rdparty/stout/include/stout/linkedhashmap.hpp <https://reviews.apache.org/r/13464/#comment49216> Not yours but hashmap returns hashsets for keys and values, can you add a TODO or update hashmap to return lists? values() should definitely not be returning a hashset there (that's totally broken since hashmaps can contain duplicate values). Not sure how much you want to fix so TODO(vinod) or TODO(bmahler) is fine if you want to delegate. :) I like what you have here, we could optionally add the following to both LinkedHashMap and hashmap, akin to python: hashset<Key> keyset() const; This would also unbreak code that relies on hashmap.keys() returning a hashset. 3rdparty/libprocess/3rdparty/stout/include/stout/linkedhashmap.hpp <https://reviews.apache.org/r/13464/#comment49217> You could use foreachvalue if you have the iterator operations, which would avoid looking up each key in the map :) If not, then can you iterate over the values directly instead of the keys? 3rdparty/libprocess/3rdparty/stout/include/stout/linkedhashmap.hpp <https://reviews.apache.org/r/13464/#comment49218> s/int/size_t/ 3rdparty/libprocess/3rdparty/stout/include/stout/linkedhashmap.hpp <https://reviews.apache.org/r/13464/#comment49208> How about keys_.empty()? 3rdparty/libprocess/3rdparty/stout/tests/linkedhashmap_tests.cpp <https://reviews.apache.org/r/13464/#comment49219> Looks like there's no need for the fixture? 3rdparty/libprocess/3rdparty/stout/tests/linkedhashmap_tests.cpp <https://reviews.apache.org/r/13464/#comment49220> expect erase to return 1 / 0 if present / missing. End comment with period. 3rdparty/libprocess/3rdparty/stout/tests/linkedhashmap_tests.cpp <https://reviews.apache.org/r/13464/#comment49221> s/5/keys.size()/ Also this check is redundant with the one below, no? - Ben Mahler On Aug. 12, 2013, 5:49 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/13464/ > ----------------------------------------------------------- > > (Updated Aug. 12, 2013, 5:49 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Repository: mesos-git > > > Description > ------- > > See summary. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/Makefile.am > 0cd407cfbd3226ce610ef77cee3a16c0e198e144 > 3rdparty/libprocess/3rdparty/stout/Makefile.am > 770378d11d30e1e222e418c1839affd9d4ffe8d3 > 3rdparty/libprocess/3rdparty/stout/include/stout/linkedhashmap.hpp > PRE-CREATION > 3rdparty/libprocess/3rdparty/stout/tests/linkedhashmap_tests.cpp > PRE-CREATION > > Diff: https://reviews.apache.org/r/13464/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
