Tim Armstrong has posted comments on this change. Change subject: Add a build flag for the undefined behavior sanitizer, aka "ubsan". ......................................................................
Patch Set 3: (13 comments) http://gerrit.cloudera.org:8080/#/c/5082/3/be/src/exec/read-write-util.cc File be/src/exec/read-write-util.cc: PS3, Line 99: PutZ PutZInteger() to be consistent with ReadZInteger() above. http://gerrit.cloudera.org:8080/#/c/5082/3/be/src/exprs/bit-byte-functions-ir.cc File be/src/exprs/bit-byte-functions-ir.cc: PS3, Line 152: rotate_left Maybe rotate_left_unsigned so that it's more immediately obvious that the arguments are unsigned. Did you do any perf tests (e.g. use one of these functions in a predicate). I suspect gcc should be able to undo all of this abstraction but don't feel 100% confident. http://gerrit.cloudera.org:8080/#/c/5082/3/be/src/exprs/math-functions-ir.cc File be/src/exprs/math-functions-ir.cc: PS3, Line 362: abs std::abs() Line 364: if (src_num == std::numeric_limits<decltype(src_num)>::min()) { Maybe we should make a utility function - UnsignedAbs() or something like that? PS3, Line 364: std::numeric_limits This shows up in a couple of places - consider putting using std::numeric_limits up the top of the file? http://gerrit.cloudera.org:8080/#/c/5082/3/be/src/runtime/raw-value.cc File be/src/runtime/raw-value.cc: Line 161: if (dest->len) memcpy(dest->ptr, src->ptr, dest->len); != 0 http://gerrit.cloudera.org:8080/#/c/5082/3/be/src/runtime/tuple.h File be/src/runtime/tuple.h: PS3, Line 76: extra space http://gerrit.cloudera.org:8080/#/c/5082/3/be/src/thirdparty/mustache/mustache.cc File be/src/thirdparty/mustache/mustache.cc: Line 361: bool is_triple = true; Would be good to get this into the upstream mustache: https://github.com/henryr/cpp-mustache/ http://gerrit.cloudera.org:8080/#/c/5082/3/be/src/util/overflow-test.cc File be/src/util/overflow-test.cc: PS3, Line 64: betwen between http://gerrit.cloudera.org:8080/#/c/5082/3/be/src/util/overflow.h File be/src/util/overflow.h: Line 94: /// This is not guaranteed to return reasonable results if the operator overflows the I'm not sure that I understand this comment. PS3, Line 95: folowing following http://gerrit.cloudera.org:8080/#/c/5082/3/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: PS3, Line 386: auto int64_t? I find this code a bit harder to follow without knowing this type. http://gerrit.cloudera.org:8080/#/c/5082/3/be/src/util/tuple-row-compare.h File be/src/util/tuple-row-compare.h: Line 99: (*codegend_compare_fn_)(key_expr_ctxs_lhs_.data(), key_expr_ctxs_rhs_.data(), lhs, rhs); Long line -- To view, visit http://gerrit.cloudera.org:8080/5082 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I88c7234bd7c5eb7404490a0913d90470c10835e7 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
