David Ribeiro Alves has posted comments on this change.

Change subject: Add a EraseKeyReturnSmartPtrValue to map-util
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/3595/1/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:

Line 700: // EraseKeyReturnSmartPtrValue(&my_map, "my_key, &my_value);
> A tiny typo: "my_key -- > "my_key"
Done


Line 701: template <class Collection, class SmartPtr>
> What if the collection is a multi-key container, like multimap?  Is it OK j
this is meant to be analogous to the method above, so yes, but left a note and 
in the case above too.


Line 702: void EraseKeyReturnSmartPtrValue(Collection* const collection,
> Consider returning the result as a smart pointer instead of placing it into
see my comment on a previous patch on pointer vs reference (which couldn't be 
const since we're mutating the map).
None of the methods in this file check the nullness of the collection, so for 
consistency, not checking it here either.
Finally changed this to return a value instead of using an out param like you 
suggested.


Line 703:                                  const typename 
Collection::value_type::first_type& key,
> Why not just Collection::key_type, but Collection::value_type::first_type?
just copy/paste reasons, changed it.


http://gerrit.cloudera.org:8080/#/c/3595/1/src/kudu/util/map-util-test.cc
File src/kudu/util/map-util-test.cc:

Line 80: TEST(EraseKeyReturnSmartPtrValueTest, TestEraseKeyReturnSmartPtrValue) 
{
> Does it make sense to add a testcase for multi-key dictionary like multimap
don't think so, since it's not the primary use case


-- 
To view, visit http://gerrit.cloudera.org:8080/3595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I65d17dd26317f77c4c0aa4b8ec00fbe51f8d1ced
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

Reply via email to