Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201610212258.u9LMw8rq022486@ip-10-146-233-104.ec2.internal>
Date: Fri, 21 Oct 2016 22:58:07 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: Jim Apple <jbapple@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
Reply-To: tarmstrong@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-3676=3A_Use_clang_as_a_static_analysis_tool=0A?=
In-Reply-To: <gerrit.1476907983000.I4ed168488cb30ddeccd0087f3840541d858f9c06@gerrit.cloudera.org>
References: <gerrit.1476907983000.I4ed168488cb30ddeccd0087f3840541d858f9c06@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Fri, 21 Oct 2016 22:58:31 -0000

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