Tim Armstrong has posted comments on this change. Change subject: Fix undefined cals to __builtin_ctz. ......................................................................
Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/5004/3/be/src/udf_samples/hyperloglog-uda.cc File be/src/udf_samples/hyperloglog-uda.cc: PS3, Line 78: auto uint64_t? Seems clearer to specify the type and it's not too verbose. PS3, Line 80: hash_top_bits hash_top_bits != 0 would make the intent clearer (it's a matter of taste, but that's generally what we do in the codebase). http://gerrit.cloudera.org:8080/#/c/5004/3/be/src/util/bit-util.h File be/src/util/bit-util.h: Line 32: #include <boost/type_traits/make_unsigned.hpp> Is this used? Line 247: unsigned int v, int otherwise = sizeof(unsigned int) * CHAR_BIT) { We don't use the optional argument, so it seems like we should just remove that generality. Would be good to explain that __builtin_ctz(0) is undefined too. -- To view, visit http://gerrit.cloudera.org:8080/5004 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple <[email protected]> Gerrit-Reviewer: Amos Bird <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
