Alexey Serbin 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" Line 701: template <class Collection, class SmartPtr> What if the collection is a multi-key container, like multimap? Is it OK just to remove the first element having that key and leave the rest? If so, please add explanation about that in the comment for the function. Line 702: void EraseKeyReturnSmartPtrValue(Collection* const collection, Consider returning the result as a smart pointer instead of placing it into the parameter placeholder. From the caller's perspective that would be more terse then: unique_ptr<MyType> p(EraseKeyReturnSmartPtrValue(my_map, "key"); Also, why not to pass input collection by const reference? Passing parameter by pointer means it might be null, so it's necessary to take care of that. Line 703: const typename Collection::value_type::first_type& key, Why not just Collection::key_type, but Collection::value_type::first_type? 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? -- 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: Kudu Jenkins Gerrit-HasComments: Yes
