Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10948 )
Change subject: IMPALA-5031: Fix undefined behavior: memset NULL ...................................................................... Patch Set 3: Code-Review+1 (2 comments) I'm fine with moving forward with this as-is, Todd's suggestion makes sense but I'll leave it up to you two http://gerrit.cloudera.org:8080/#/c/10948/3/be/src/util/ubsan.h File be/src/util/ubsan.h: http://gerrit.cloudera.org:8080/#/c/10948/3/be/src/util/ubsan.h@26 PS3, Line 26: class Ubsan { > I think in those cases I'd probably put each of the methods into wherever t Todd's suggestion makes sense to me. I don't think this way of organising utilities is going to cause any significant issues though. http://gerrit.cloudera.org:8080/#/c/10948/2/be/src/util/ubsan.h File be/src/util/ubsan.h: http://gerrit.cloudera.org:8080/#/c/10948/2/be/src/util/ubsan.h@29 PS2, Line 29: if (s == nullptr) { > I picked the first one, because in a release build, the calling MemSet(NULL In principle I don't agree with writing code to defensively handle invariant violations. In this case it doesn't complicate the code but I feel like maybe this is part of the reason we had trouble getting the ubsan changes through. IMO, we should add the appropriate DCHECKs and tests and then streamline the code as much as possible. It's impossible to test handling of invariant violations by definition and once you start doing it there's an endless number of opportunities to complicate code by being unnecessarily defensive. -- To view, visit http://gerrit.cloudera.org:8080/10948 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I18fa02dc887a42a94c6f81e4923d17568f2184f2 Gerrit-Change-Number: 10948 Gerrit-PatchSet: 3 Gerrit-Owner: Jim Apple <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 18 Jul 2018 20:55:10 +0000 Gerrit-HasComments: Yes
