Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12866 )
Change subject: util: helper class for working with bitsets ...................................................................... Patch Set 5: (8 comments) http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h File src/kudu/util/bitset.h: http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@33 PS4, Line 33: compile > 'compile' or 'compilation' Done http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@40 PS4, Line 40: size_t MaxVals > Could MaxVals be deduced based on IntType? IntType stands for the enum's t I don't think so, I haven't seen a way that can do it without defining MaxVals somehow (e.g as another enum value, or as a constexpr value). Right, IntType is the enum type. http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@46 PS4, Line 46: typedef IntType key_type; > Is it necessary to have it in the public section? Maybe, leave it private? Yep, these are publicly needed for use in map-util.h http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@68 PS4, Line 68: const_iterator > I'm curious when it's necessary to have a reference to the 'inserted' eleme This is just to match the STL interface, http://www.cplusplus.com/reference/set/set/insert/ The iterator itself isn't particularly useful (for my purposes), but I matched the interface so this could be used with InsertOr*() methods which leverage it. http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@105 PS4, Line 105: size_t size() const { > const? Done http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@113 PS4, Line 113: IntType& > I guess in case of small enums using value instead of reference might be be I put these into godbolt https://godbolt.org/z/btGPzW and it seems compilers are smart enough to generate the same code for small enums. http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@131 PS4, Line 131: 'va > What's 'i'? Whoops, renamed it. http://gerrit.cloudera.org:8080/#/c/12866/4/src/kudu/util/bitset.h@184 PS4, Line 184: if (idx_ == MaxVals) { > I'm not sure this is safe, even if this method is private. What happens if Ah, good catch. -- 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: 5 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 27 Mar 2019 04:24:53 +0000 Gerrit-HasComments: Yes
