> On Aug. 13, 2013, 2:15 a.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/linkedhashmap.hpp, line 8 > > <https://reviews.apache.org/r/13464/diff/4/?file=340073#file340073line8> > > > > s/hashmap/HashMap/ then? ;) > > > > Eventually we can rename stout's data structures to use CamelCase.
yup, from talking to BenH that was the plan. we can do the renaming of hashmap (if necessary) then. > On Aug. 13, 2013, 2:15 a.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/linkedhashmap.hpp, lines > > 15-16 > > <https://reviews.apache.org/r/13464/diff/4/?file=340073#file340073line15> > > > > Can you move these typedefs down into the private block where they are > > used? They are used in the public methods. FWIW, this is the sytle followed by stout::cache and boost. > On Aug. 13, 2013, 2:15 a.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/linkedhashmap.hpp, line 12 > > <https://reviews.apache.org/r/13464/diff/4/?file=340073#file340073line12> > > > > 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 :) In the interest of time, I've added a TODO for extending from hashmap and adding iterators. > On Aug. 13, 2013, 2:15 a.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/linkedhashmap.hpp, lines > > 27-37 > > <https://reviews.apache.org/r/13464/diff/4/?file=340073#file340073line27> > > > > 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 i added that before i added []. killed 'put' since i never really used it outside tests. > On Aug. 13, 2013, 2:15 a.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/linkedhashmap.hpp, lines > > 59-69 > > <https://reviews.apache.org/r/13464/diff/4/?file=340073#file340073line59> > > > > 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. Added a TODO in hashmap. I don't see a reason for returning hashsets in linkedhashmap because the whole point of it is get keys and values in the insertion order. If we need it later for some reason, I prefer to add it then. > On Aug. 13, 2013, 2:15 a.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/linkedhashmap.hpp, lines > > 65-66 > > <https://reviews.apache.org/r/13464/diff/4/?file=340073#file340073line65> > > > > 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? I can't iterate on values because 'keys' contains keys in the insertion order. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13464/#review25049 ----------------------------------------------------------- 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 > >
