impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Date Fri, 21 Oct 2016 22:58:07 GMT
Tim Armstrong has posted comments on this change.

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


Patch Set 4:

(7 comments)

Just a few remaining things.

http://gerrit.cloudera.org:8080/#/c/4758/2/be/CMakeLists.txt
File be/CMakeLists.txt:

Line 106: SET(CXX_FLAGS_TIDY "${CXX_FLAGS_TIDY} --gcc-toolchain=${GCC_ROOT}")
> I think that might be just for building the toolchain. I tried changing it 
It sets up CMake to use clang as the C++ compiler (see bin/impala_impala.sh where the different
build types use different toolchain.cmake).

We already have all of the logic in there to build Impala with Clang so we should use that
instead of solving it in a different way for this build type.


http://gerrit.cloudera.org:8080/#/c/4758/4/be/src/common/status.h
File be/src/common/status.h:

Line 212:     return *msg_; // NOLINT: with DCHECKS off, this might deref a nullptr
I still think we should reword this to make it clear that it's a limitation of the static
analysis not a product bug, e.g.

// NOLINT: clang-tidy thinks this might deref a nullptr.


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/runtime/coordinator.h
File be/src/runtime/coordinator.h:

> I'm interested in your thoughts on which struct reordering s are, in your o
Either if the multiple instances of the struct are touched on a particularly perf-critical
part of query execution (e.g. maybe BloomFilter) or if there are so many instances of it that
they make up a significant part of the memory footprint of the process (hash table buckets,
tuples, etc).

We only have one coordinator per query and it shouldn't be touched that frequently (only when
returning each result batch or serving an RPC) so I don't see a benefit.


http://gerrit.cloudera.org:8080/#/c/4758/4/be/src/runtime/coordinator.h
File be/src/runtime/coordinator.h:

Line 432:   bool needs_finalization_;
Can you revert these changes?


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:

Line 335
> I have put these variables back in the logical (but suboptimaly packed) ord
I'm ok with this but we should revisit if there are a lot of spurious warnings or people feel
compelled to repack everything.


http://gerrit.cloudera.org:8080/#/c/4758/4/bin/run_clang_tidy.sh
File bin/run_clang_tidy.sh:

Line 46:     # This script has been tested when CORES is actually higher than the number of
cores
2 space indents are the (imperfectly followed) standard in the shell scripts.


http://gerrit.cloudera.org:8080/#/c/4758/4/buildall.sh
File buildall.sh:

Line 256: 
Remove extra blank line.


-- 
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jbapple@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message