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 2: (10 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@34 PS1, Line 34: > warning: using decl 'vector' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset-test.cc@44 PS1, Line 44: typedef set<TestEnum> EnumSet; > warning: 'MaxEnumVal' is a static definition in anonymous namespace; static Done http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset-test.cc@96 PS1, Line 96: > Would be good to get some FindOr* coverage too. FindOr* doesn't work for set types by virtue of them not being a pair. It also doesn't seem to have any value over using ContainsKey() for sets. http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset-test.cc@128 PS1, Line 128: // Do a final sanity check that the bitset looks how we expect. > Check out https://github.com/google/googletest/blob/master/googletest/docs/ Forks :pepe-hands: 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: #include <glog/logging.h> > You should also talk about MaxVals; I think the "static" aspect of the Fixe Done http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h@30 PS1, Line 30: s useful for building containers for enum types, : // instead of usi > Should we enforce this via COMPILE_ASSERT or the like? Done http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h@39 PS1, Line 39: // compile time. > Don't think you need the template arguments in the constructors. Done http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h@43 PS1, Line 43: COMPILE_ASSERT(MaxVals < 64, bitset_size_too_large); > warning: single-argument constructors must be marked explicit to avoid unin Done http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h@60 PS1, Line 60: template <typename Container> > To make this more STL-like, should this return an integer indicating the nu Done http://gerrit.cloudera.org:8080/#/c/12866/1/src/kudu/util/bitset.h@66 PS1, Line 66: > This isn't really an STL method; find() should be the primary implementatio The implementation is easier with this method, not having to keep comparing against end() and whatnot. FWIW it will be in C++20. https://en.cppreference.com/w/cpp/container/set/contains -- 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: 2 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[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 00:30:49 +0000 Gerrit-HasComments: Yes
