Jim Apple has posted comments on this change. Change subject: IMPALA-3676: Use clang as a static analysis tool ......................................................................
Patch Set 7: (8 comments) > (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. Done > > 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? I rearranged them all. Tim convinced me to roll some back. 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 (simi While it can, clang-format "fixes" it to this right away. 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? Accident. PS6, Line 176: > why remove this? Put it back in the definition in the class. http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/exprs/aggregate-functions.h File be/src/exprs/aggregate-functions.h: PS6, Line 182: ec > can you replace that with HLL_PRECISION? Done 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: uniqu > remove std:: in cc files Removed here and elsewhere, except for std::move. I see that usually referred to with the prefix by C++ people. Thoughts? http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/statestore/statestore.h File be/src/statestore/statestore.h: PS6, Line 82: CacheLineAligned > why? subscriber_heartbeat_threadpool_ is a ThreadPool<T>, and ThreadPool<T>::work_queue_ is a BlockingQueue<S>, and BlcokingQueue<S> is marked CACHE_ALIGNED, presumably to avoid false sharing. I made CacheLineAligned use the C++11 alignas specifier and deleted CACHE_ALIGNED. PS6, Line 253: boost::mutex exit_flag_lock_; > This is a good example of where logical grouping is broken by grouping-by-t rolled back 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 should be allocate > What does 'must be allocated' mean - for correctness or performance or both either - performance for BlockingQueue and correctness for SIMD values when using aligned loads or stores. -- 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: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
