Alex Behm has posted comments on this change. Change subject: IMPALA-2809: Improve scalar ByteSwap(). ......................................................................
Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/3033/3/be/src/util/bit-util.h File be/src/util/bit-util.h: Line 158: return static_cast<uint32_t>(__builtin_bswap32(value)); > I don't understand why we have casts here. AFAIK, bswap32 should return uns You are correct. The builtin bswap returns unsigned, and the casts here don't make sense. Removed casts. Line 164: return static_cast<uint16_t>(ByteSwap(static_cast<int16_t>(value))); > if 16-bit byteswap is a common case, it might be worth seeing if we get bet There's a __builtin_bswap16, so just using that now, http://gerrit.cloudera.org:8080/#/c/3033/3/be/src/util/bit-util.inline.h File be/src/util/bit-util.inline.h: Line 24: uint8_t* dst = reinterpret_cast<uint8_t*>(dest); > why unsigned here but signed everywhere else? Using unsigned everywhere now. Line 28: *reinterpret_cast<int8_t*>(dst) = *reinterpret_cast<const int8_t*>(src); > why signed rather than unsigned? for byte manipulation, unsigned makes more Using unsigned everywhere now. Line 35: *reinterpret_cast<int16_t*>(dst + 1) = > these loads/stores will be unaligned. on modern x86 that should be okay fro Agreed. Line 118: return; > are all these cases exercised pretty well by impala? if not a simple unit t Added unittest. -- To view, visit http://gerrit.cloudera.org:8080/3033 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4f462e6bdb022db46b48889a6a7426120a80d9b4 Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
