Jim Apple has posted comments on this change. Change subject: IMPALA-4123: Fast bit unpacking ......................................................................
Patch Set 7: (10 comments) found with help of clang-tidy http://gerrit.cloudera.org:8080/#/c/4494/7/be/src/benchmarks/bit-packing-benchmark.cc File be/src/benchmarks/bit-packing-benchmark.cc: PS7, Line 330: int argc, char **argv unused http://gerrit.cloudera.org:8080/#/c/4494/7/be/src/util/bit-packing.h File be/src/util/bit-packing.h: Line 22: #ifndef IMPALA_UTIL_BIT_PACKING_H These usually go above the #includes PS7, Line 48: BitPacking Why not put this into use in this patch? In other words, why only the microbenchmark? http://gerrit.cloudera.org:8080/#/c/4494/7/be/src/util/bit-packing.inline.h File be/src/util/bit-packing.inline.h: PS7, Line 45: make_ make_ can be omitted. PS7, Line 90: onstants constants PS7, Line 115: else if branch returns, so you can drop the "else" here. PS7, Line 118: else else if branch returns, so you can drop the else here, if you want to. I'm ambivalent. Line 120: DCHECK_LT(VALUE_IDX, 31) << "Trailing bits after last word."; Can you explain more why VALUE_IDX < 31? Can you add a static_assert at the top with the maximum value it could possibly be? PS7, Line 126: warning What template values cause that warning to trigger? PS7, Line 184: BIT_WIDTH if BIT_WIDTH is 0, I don't think this is technically allowed. -- To view, visit http://gerrit.cloudera.org:8080/4494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622 Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
