Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11401 )
Change subject: [gutil] add LookupOrEmplace() ...................................................................... Patch Set 1: (3 comments) 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 That's not always the case: it depends on libc++ version. The right wording here is 'maybe used up'. 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)` i The reason is simple: ASSERT_NE is a much better way to handle a failure in the dedicated test compared with crash on SIGABRT. 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 t I'll better just remove this. The reason is that on some older libc++ implementations this assertion does not hold, and I don't think it's worth finding the version range for those. -- 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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Mon, 10 Sep 2018 16:56:09 +0000 Gerrit-HasComments: Yes
