Tim Armstrong has posted comments on this change. Change subject: Add a build flag for the undefined behavior sanitizer, aka "ubsan". ......................................................................
Patch Set 4: Code-Review+1 (4 comments) Just a few minor things from my point of view. It seems like there was some discussion around the best way to do the casts (i.e. whether to just use the builtin cast operators). I think the current approach works nicer inside templated functions because there's less boilerplate like make_unsigned. http://gerrit.cloudera.org:8080/#/c/5082/4/be/src/util/overflow.h File be/src/util/overflow.h: PS4, Line 101: underfined undefined Line 121: static auto CheckedSum(T x, T y, bool* overflow) { always_inline? Line 132: static auto CheckedProduct(T x, U y, bool * overflow) { always_inline? This function seems around the size where inlining heuristics might be a bit unstable and it would be easy for a minor change to alter the inlining and perf. Line 152: if (std::is_signed<T>::value && x == std::numeric_limits<T>::min()) { Can is_signed should be a precondition of this function? It doesn't do anything on unsigned numbers. I spent a bit of time thinking about whether we can implement this more efficiently but I didn't see an obviously better solution. The branch should be rarely taken so it's probably better than a solution that would have a less predictable branch like x < 0. -- 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: 4 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: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
