impala-reviews mailing list archives

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

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

Patch Set 6:


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?
File be/src/common/

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

PS6, Line 173: 
Why remove these comments?

PS6, Line 176: 
why remove this?
File be/src/exprs/aggregate-functions.h:

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

PS6, Line 140: std::
remove std:: in cc files
File be/src/statestore/statestore.h:

PS6, Line 82: CacheLineAligned

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4ed168488cb30ddeccd0087f3840541d858f9c06
Gerrit-PatchSet: 6
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