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,4321: Use clang as a static analysis tool
Date Wed, 19 Oct 2016 20:41:04 GMT
Jim Apple has posted comments on this change.

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

Patch Set 1:

Commit Message:

Line 18: This patch also fixes a number of small bugs found by clang-tidy.
I am running the full test suite on this patch now.
File be/src/benchmarks/

Line 232:       for (int log10fpp = -1; log10fpp >= -3; --log10fpp) {
Using floating point numbers as loop variables is not necessarily reliable.
File be/src/benchmarks/

Line 165: #pragma clang diagnostic push
#including .cc files ends up with hidden 'using namespace' directives.
File be/src/benchmarks/

Line 72
File be/src/catalog/catalog-server.h:

Line 154:   [[noreturn]] void GatherCatalogUpdatesThread();
File be/src/codegen/

Line 308
dead code
File be/src/common/logging.h:

PS1, Line 45: 
It does not define this, and it would be illegal to do so, as identifiers staring with _ are
reserved for the implementation.
File be/src/common/names.h:

Line 35: #if defined(_GLIBCXX_VECTOR) || defined(_LIBCPP_VECTOR)
Make this file work with libc++, the clang standard library implementation
File be/src/common/status.h:

Line 263
This is taken care of by the return value optimization and the named return value optimization.
File be/src/exec/delimited-text-parser.h:

Line 168
Adds useless padding bytes to the type
File be/src/exec/exec-node.h:

Line 28
I was in here anyway, so alphabetize the headers as our style guide says
File be/src/exec/external-data-source-executor.h:

PS1, Line 37: 
useless ;
File be/src/exec/

PS1, Line 478: 
never used
File be/src/exec/hbase-scan-node.h:

PS1, Line 55: 
const on return values is mostly meaningless
File be/src/exec/

Line 86:   Function* materialize_tuple_fn = NULL;
ensure initialization along all paths

Line 639:         success = false;
when DCHECKs are off, ensure initialization
File be/src/exec/hdfs-scan-node.h:

Line 159
never used
File be/src/exec/

Line 53
NRVO and RVO again
File be/src/exec/

Line 20: #include <numeric>
For accumulate
File be/src/experiments/bit-stream-utils.8byte.inline.h:

Line 100:     if (num_bits - bit_offset_ < size) {
handle undefined behavior when shift too large
File be/src/experiments/

Line 36
File be/src/exprs/

Line 6047
clarify suspicious unsigned shift
File be/src/exprs/

Line 676:     stringstream ss;
url_part was leaking in this block
File be/src/rpc/TAcceptQueueServer.h:

PS1, Line 22: 
implementation-reserved identifier
File be/src/rpc/

Line 29: #include <random>
File be/src/rpc/thrift-server.h:

Line 87:     virtual ~ConnectionHandlerIf() = default;
Objects with virtual methods should have a virtual destructor
File be/src/runtime/

Line 657
File be/src/runtime/

Line 63: class DataStreamSender::Channel : public CacheLineAligned {
For the ThreadPool
File be/src/runtime/descriptors.h:

PS1, Line 22: 
tr1 is not part of C++14
File be/src/runtime/

Line 51:     EXPECT_EQ(p1, p2);
If p1 != p2, then ASSERT exists and p2 can leak

Line 194:   pool.Free(ptr5);
If ptr4 != ptr5, ptr5 can leak
File be/src/runtime/

Line 60:     Status s = JniUtil::GetJniExceptionMsg(env, true, "HBaseTable::Close(): ");
This was a more important one than some of the others - the const char[] was getting implicitly
cast to a bool!
File be/src/runtime/

Line 81:   int128_t x = 0;
quiet the initialization warnings
File be/src/runtime/tmp-file-mgr.h:

Line 195:       const TUniqueId& query_id, std::unique_ptr<File>* new_file);
Help prevent memory leaks
File be/src/statestore/statestore.h:

Line 38: #include "util/aligned-new.h"
While I'm here, order #includes the how our style guide instructs
File be/src/util/

Line 209:     for (double fpp = 0.1; fpp > pow(2, -20); fpp *= 0.99) { // NOLINT: loop
on double
Here no int would work quite the same way
File be/src/util/buffer-builder.h:

Line 39:   inline void Append(const void* buffer, int len) __attribute__((nonnull)) {
quiet some warnings about memcpy. This specifies that buffer is not NULL
File be/src/util/

Line 44
There is also a std::error_code
File be/src/util/

Line 54
Already #included
File be/src/util/

Line 97
Not standard comformant, apparently
File be/src/util/

PS1, Line 55: 
Not an escape sequence
File be/src/util/

Line 107:   uint8_t buffer[(len > 0) ? len : 1];
0-length buffers are technically not standards compliant
File be/src/util/

Line 425:   sq_printf(connection, headers.c_str(), (int)str.length());
printf with non-literal format is a security concern, but it should be OK here
File bin/

Line 357:       "googletest",
Upgrade gtest to googletest (new name, already in the toolchain repo and s3 bucket)

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I4ed168488cb30ddeccd0087f3840541d858f9c06
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-HasComments: Yes

View raw message