Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11267 )
Change subject: common: add equality methods to ColumnBlock and SelectionVector ...................................................................... Patch Set 2: Code-Review+1 (4 comments) looks good, just nits http://gerrit.cloudera.org:8080/#/c/11267/2/src/kudu/common/columnblock.h File src/kudu/common/columnblock.h: http://gerrit.cloudera.org:8080/#/c/11267/2/src/kudu/common/columnblock.h@107 PS2, Line 107: bool Equals(const ColumnBlock& other) const { see comment in the other header file about operator== http://gerrit.cloudera.org:8080/#/c/11267/2/src/kudu/common/columnblock.h@114 PS2, Line 114: ^ nit: as a non-hardware engineer, I have trouble interpreting XOR before noon; how about something like: if ((null_bitmap_ == nullptr) != (other.null_bitmap_ == nullptr)) { http://gerrit.cloudera.org:8080/#/c/11267/2/src/kudu/common/columnblock.h@256 PS2, Line 256: 0 nit: how about: BitmapChangeBits(null_bitmap(), /*offset=*/0, n_rows, /*value=*/false); http://gerrit.cloudera.org:8080/#/c/11267/2/src/kudu/common/rowblock.h File src/kudu/common/rowblock.h: http://gerrit.cloudera.org:8080/#/c/11267/2/src/kudu/common/rowblock.h@122 PS2, Line 122: bool Equals(const SelectionVector& other) const; How about operator== ? The style guide recommendation has changed over the years and it now says to prefer == to Equals() -- To view, visit http://gerrit.cloudera.org:8080/11267 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9712bd7748bb01af7b6f68897a453a0aa149cdcf Gerrit-Change-Number: 11267 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Mon, 27 Aug 2018 19:06:08 +0000 Gerrit-HasComments: Yes
