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 <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
