Zach Amsden has posted comments on this change. Change subject: IMPALA-5266 Impala ABM / LZCNT support ......................................................................
Patch Set 8: (3 comments) Thanks for taking a look! http://gerrit.cloudera.org:8080/#/c/5821/8/be/src/util/bit-util-test.cc File be/src/util/bit-util-test.cc: PS8, Line 300: EXPECT_EQ(BitUtil::RoundUpToPowerOfTwo(7), 8); : EXPECT_EQ(BitUtil::RoundUpToPowerOfTwo(8), 8); : EXPECT_EQ(BitUtil::RoundUpToPowerOfTwo(65535), 65536); > Duplicate of L297-299. I guess the intent here was to call RoundUpToPowerOf Original implementation required user specified bit width, so this was testing 32 and 64 bit operators separately. Will get rid of it. PS8, Line 308: EXPECT_EQ(BitUtil::RoundUpToPowerOfTwoNoHw(7), 8); : EXPECT_EQ(BitUtil::RoundUpToPowerOfTwoNoHw(8), 8); : EXPECT_EQ(BitUtil::RoundUpToPowerOfTwoNoHw(65535), 65536); > same here Done http://gerrit.cloudera.org:8080/#/c/5821/8/be/src/util/bit-util.h File be/src/util/bit-util.h: Line 376: DCHECK(v > 0); > DCHECK on L389 asserts that RoundUpToPowerOfTwo(int32_t) never overflows. D I would argue we don't need that check for 64-bit math. -- To view, visit http://gerrit.cloudera.org:8080/5821 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2 Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-HasComments: Yes
