Henry Robinson has posted comments on this change. Change subject: IMPALA-3676: Use clang as a static analysis tool ......................................................................
Patch Set 6: (8 comments) The commit message doesn't really tell the full story here. In particular, you should say something about AlignmentNew and what it is for. The rearrangement of class members to group by type seems like it sacrifices readability for performance in some strange cases - the statestore is a good example, where there is only ever one statestore and it is not (as far as we know) highly sensitive to its in-memory layout. What criteria did you use to decide that a class' members should be grouped-by-type? http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/common/init.cc File be/src/common/init.cc: PS6, Line 100: [[noreturn]] can this go on the previous line? It might be easier to read that way (similar to how we do template<> declarations). http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: PS6, Line 173: : Why remove these comments? PS6, Line 176: why remove this? http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/exprs/aggregate-functions.h File be/src/exprs/aggregate-functions.h: PS6, Line 182: 10 can you replace that with HLL_PRECISION? http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/runtime/tmp-file-mgr.cc File be/src/runtime/tmp-file-mgr.cc: PS6, Line 140: std:: remove std:: in cc files http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/statestore/statestore.h File be/src/statestore/statestore.h: PS6, Line 82: CacheLineAligned why? PS6, Line 253: boost::mutex exit_flag_lock_; This is a good example of where logical grouping is broken by grouping-by-type. The lock and the flag are related, but are not next to each other. http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/util/aligned-new.h File be/src/util/aligned-new.h: PS6, Line 27: Objects that must be allocated What does 'must be allocated' mean - for correctness or performance or both? -- To view, visit http://gerrit.cloudera.org:8080/4758 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4ed168488cb30ddeccd0087f3840541d858f9c06 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple <jbap...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbap...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
