Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11401 )
Change subject: [gutil] add LookupOrEmplace() ...................................................................... Patch Set 1: (3 comments) The change looks fine with me, though I think Adar may the same change lying around somewhere. See https://gerrit.cloudera.org/c/11303/4/src/kudu/gutil/map-util.h#464 for a discussion we had about it and emplace semantics. http://gerrit.cloudera.org:8080/#/c/11401/1/src/kudu/gutil/map-util.h File src/kudu/gutil/map-util.h: http://gerrit.cloudera.org:8080/#/c/11401/1/src/kudu/gutil/map-util.h@498 PS1, Line 498: // It's similar to LookupOrInsert() but uses the emplace and r-value mechanics : // to achive the desired results. Worth noting that if there is an element with the key already exists in the container, the arguments will still be "used up". "The element may be constructed even if there already is an element with the key in the container, in which case the newly constructed element will be destroyed immediately." >From https://en.cppreference.com/w/cpp/container/map/emplace http://gerrit.cloudera.org:8080/#/c/11401/1/src/kudu/util/map-util-test.cc File src/kudu/util/map-util-test.cc: http://gerrit.cloudera.org:8080/#/c/11401/1/src/kudu/util/map-util-test.cc@133 PS1, Line 133: auto* val_ptr = FindOrNull(int_map, key); : ASSERT_NE(nullptr, val_ptr); Is there a particular reason to use `FindOrNull()` + `ASSERT_NE(nullptr)` instead of `FindOrDie()`? To introduce test coverage for `FindOrNull()` perhaps? http://gerrit.cloudera.org:8080/#/c/11401/1/src/kudu/util/map-util-test.cc@171 PS1, Line 171: //ASSERT_NE(nullptr, val.get()); Uncomment? Below too. Also see my comment in the header about documenting these semantics. -- To view, visit http://gerrit.cloudera.org:8080/11401 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3a177f410c1d4ed33e6f44cb7d6c33d6141f84fc Gerrit-Change-Number: 11401 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Mon, 10 Sep 2018 03:16:37 +0000 Gerrit-HasComments: Yes
