Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15539 )
Change subject: bitmap: faster implementation of iteration over a bitmap ...................................................................... Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/15539/1/src/kudu/util/bitmap-inl.h File src/kudu/util/bitmap-inl.h: http://gerrit.cloudera.org:8080/#/c/15539/1/src/kudu/util/bitmap-inl.h@17 PS1, Line 17: #pragma once Curious why not implement these functions in existing bitmap.h? http://gerrit.cloudera.org:8080/#/c/15539/1/src/kudu/util/bitmap-inl.h@30 PS1, Line 30: int64_t Why not uint64_t here? http://gerrit.cloudera.org:8080/#/c/15539/1/src/kudu/util/bitmap-inl.h@59 PS1, Line 59: break; just return here instead? http://gerrit.cloudera.org:8080/#/c/15539/1/src/kudu/util/bitmap-test.cc File src/kudu/util/bitmap-test.cc: http://gerrit.cloudera.org:8080/#/c/15539/1/src/kudu/util/bitmap-test.cc@37 PS1, Line 37: & Nit: Just capture result by reference. http://gerrit.cloudera.org:8080/#/c/15539/1/src/kudu/util/bitmap-test.cc@87 PS1, Line 87: set Is ordered set necessary? http://gerrit.cloudera.org:8080/#/c/15539/1/src/kudu/util/bitmap-test.cc@109 PS1, Line 109: const Nit: constexpr http://gerrit.cloudera.org:8080/#/c/15539/1/src/kudu/util/bitmap-test.cc@110 PS1, Line 110: const constexpr http://gerrit.cloudera.org:8080/#/c/15539/1/src/kudu/util/bitmap-test.cc@118 PS1, Line 118: & Only sum by reference -- To view, visit http://gerrit.cloudera.org:8080/15539 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibe772a7dd92faf9f99115148ad4cc7df542d1c76 Gerrit-Change-Number: 15539 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 24 Mar 2020 00:43:32 +0000 Gerrit-HasComments: Yes
