Alexey Serbin has posted comments on this change. Change subject: Add ComputeIfAbsent methods to map-util ......................................................................
Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/3593/5/src/kudu/gutil/map-util.h File src/kudu/gutil/map-util.h: Line 792: pair<typename MapContainer::value_type::second_type* const, bool> Why not just MapContainer::mapped_type, but MapContainer::value_type::first_type? Line 793: ComputeIfAbsentReturnAbsense(MapContainer* container, > it's due to code style. we pass by pointer when we'll be mutating the conta OK, code style is code style, whether it's ugly or not. But if using pointers, why there is no check for a nullptr then? Is this ok to get SIGSEGV when calling this function with a null pointer as a parameter or there should be some code which checks for null before de-referencing it? Line 794: const typename MapContainer::value_type::first_type& key, MapContainer::value_type::first_type --> MapContainer::key_type? Line 814: typename MapContainer::value_type::second_type* const MapContainer::mapped_type? Line 815: ComputeIfAbsent(MapContainer* container, Does in make sense to reflect the fact the value is being inserted? I.e., name this InsertComputedIfAbsent() Line 816: const typename MapContainer::value_type::first_type& key, MapContainer::value_type::first_type --> MapContainer::key_type? http://gerrit.cloudera.org:8080/#/c/3593/5/src/kudu/util/map-util-test.cc File src/kudu/util/map-util-test.cc: Line 58: TEST(ComputeIfAbsentTest, TestComputeIfAbsentAntReturnAbsense) { typo? TestComputeIfAbsentAntReturnAbsense --> TestComputeIfAbsentAndReturnAbsense Or just rename it into TestComputeIfAbsentReturnAbsense -- To view, visit http://gerrit.cloudera.org:8080/3593 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba9fa8038e699e66d34ce541cd02c77f46691315 Gerrit-PatchSet: 5 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-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
