impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jim Apple (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Date Mon, 24 Oct 2016 21:26:23 GMT
Jim Apple has posted comments on this change.

Change subject: IMPALA-3676: Use clang as a static analysis tool

Patch Set 7:


 > 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?

I rearranged them all. Tim convinced me to roll some back.
File be/src/common/

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.
File be/src/exprs/

PS6, Line 173: 
> Why remove these comments?

PS6, Line 176: 
> why remove this?
Put it back in the definition in the class.
File be/src/exprs/aggregate-functions.h:

PS6, Line 182: ec
> can you replace that with HLL_PRECISION?
File be/src/runtime/

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?
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
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I4ed168488cb30ddeccd0087f3840541d858f9c06
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message