Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15043 )
Change subject: encoding-test: Clean up bitshuffle tests a little ...................................................................... Patch Set 9: (7 comments) http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random.h File src/kudu/util/random.h: http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random.h@91 PS9, Line 91: upto > nit: missing space Done http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random.h@236 PS9, Line 236: Next128 > nit: for consistency, add () Done http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random.h@247 PS9, Line 247: return sizeof(IntType) == 4 ? rand->Next32() : : sizeof(IntType) == 8 ? rand->Next64() : rand->Next128(); > nit: may be simpler with a switch. Same below This is a constexpr function so can't use if/switch in C++11. See comment on line 242. Moved the comment right above this line for better readability. http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random.h@252 PS9, Line 252: integer > nit: maybe specify that this can be signed or unsigned Done http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h File src/kudu/util/random_util.h: http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h@128 PS9, Line 128: > nit: add a comma; same elsewhere. Oxford comma is optional unless our coding guidelines explicitly require it. So I'm going to drop this comment. http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h@131 PS9, Line 131: std::vector<IntType> CreateRandomIntegersInRange(const int num_values, > Could you also add a small test for this in util/random_util-test.cc? Done http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h@146 PS9, Line 146: exception > What exception is this? This is the comment for ASAN exception added for this function. See L130. -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 9 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 08 Apr 2020 22:11:37 +0000 Gerrit-HasComments: Yes
