Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12866 )
Change subject: util: helper class for working with bitsets ...................................................................... Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset-test.cc File src/kudu/util/bitset-test.cc: http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset-test.cc@96 PS1, Line 96: ASSERT_EQ(InsertIfNotPresent(&enum_set, e), Would be good to get some FindOr* coverage too. http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset-test.cc@128 PS1, Line 128: TEST(TestBitSet, TestInvalidUsage) { Check out https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#death-test-naming http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h File src/kudu/util/bitset.h: http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h@25 PS1, Line 25: // Utility template for working with a bitset to make it feel more like a You should also talk about MaxVals; I think the "static" aspect of the FixedBitSet is possibly it's most important aspect (i.e. you need to know the size of the bitset at compile time to use this). http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h@30 PS1, Line 30: so the expectation is that 'MaxVals' is smaller than : // the word-size. Should we enforce this via COMPILE_ASSERT or the like? http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h@39 PS1, Line 39: FixedBitSet<IntType, MaxVals>() {} Don't think you need the template arguments in the constructors. http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h@60 PS1, Line 60: void erase(const IntType val) { To make this more STL-like, should this return an integer indicating the number of values erased (i.e. 0 or 1)? http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h@66 PS1, Line 66: bool contains(const IntType& val) const { This isn't really an STL method; find() should be the primary implementation, at which point the gutil/map-util.h methods ought to work as wrappers. -- To view, visit http://gerrit.cloudera.org:8080/12866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie0344fe94e9f9da9651164cb1b456c92d99dbdf4 Gerrit-Change-Number: 12866 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 26 Mar 2019 22:39:58 +0000 Gerrit-HasComments: Yes
