impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jim Apple (Code Review)" <ger...@cloudera.org>
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:

(44 comments)

http://gerrit.cloudera.org:8080/#/c/4758/1//COMMIT_MSG
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.


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/benchmarks/bloom-filter-benchmark.cc
File be/src/benchmarks/bloom-filter-benchmark.cc:

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


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/benchmarks/in-predicate-benchmark.cc
File be/src/benchmarks/in-predicate-benchmark.cc:

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


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/benchmarks/parse-timestamp-benchmark.cc
File be/src/benchmarks/parse-timestamp-benchmark.cc:

Line 72
unused


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/catalog/catalog-server.h
File be/src/catalog/catalog-server.h:

Line 154:   [[noreturn]] void GatherCatalogUpdatesThread();
http://en.cppreference.com/w/cpp/language/attributes


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/codegen/codegen-anyval.cc
File be/src/codegen/codegen-anyval.cc:

Line 308
dead code


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/common/logging.h
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.


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/common/names.h
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


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

Line 263
This is taken care of by the return value optimization and the named return value optimization.


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/exec/delimited-text-parser.h
File be/src/exec/delimited-text-parser.h:

Line 168
Adds useless padding bytes to the type


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/exec/exec-node.h
File be/src/exec/exec-node.h:

Line 28
I was in here anyway, so alphabetize the headers as our style guide says


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/exec/external-data-source-executor.h
File be/src/exec/external-data-source-executor.h:

PS1, Line 37: 
useless ;


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/exec/hash-table-test.cc
File be/src/exec/hash-table-test.cc:

PS1, Line 478: 
never used


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/exec/hbase-scan-node.h
File be/src/exec/hbase-scan-node.h:

PS1, Line 55: 
const on return values is mostly meaningless


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/exec/hdfs-avro-scanner.cc
File be/src/exec/hdfs-avro-scanner.cc:

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


Line 639:         success = false;
when DCHECKs are off, ensure initialization


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/exec/hdfs-scan-node.h
File be/src/exec/hdfs-scan-node.h:

Line 159
never used


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/exec/partitioned-hash-join-builder-ir.cc
File be/src/exec/partitioned-hash-join-builder-ir.cc:

Line 53
NRVO and RVO again


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/exec/partitioned-hash-join-builder.cc
File be/src/exec/partitioned-hash-join-builder.cc:

Line 20: #include <numeric>
For accumulate


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/experiments/bit-stream-utils.8byte.inline.h
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


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/experiments/tuple-splitter-test.cc
File be/src/experiments/tuple-splitter-test.cc:

Line 36
unused


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

Line 6047
clarify suspicious unsigned shift


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/exprs/string-functions-ir.cc
File be/src/exprs/string-functions-ir.cc:

Line 676:     stringstream ss;
url_part was leaking in this block


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

PS1, Line 22: 
implementation-reserved identifier


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:

Line 29: #include <random>
std::random_device


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/rpc/thrift-server.h
File be/src/rpc/thrift-server.h:

Line 87:     virtual ~ConnectionHandlerIf() = default;
Objects with virtual methods should have a virtual destructor


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

Line 657
unused


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/runtime/data-stream-sender.cc
File be/src/runtime/data-stream-sender.cc:

Line 63: class DataStreamSender::Channel : public CacheLineAligned {
For the ThreadPool


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

PS1, Line 22: 
tr1 is not part of C++14


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/runtime/free-pool-test.cc
File be/src/runtime/free-pool-test.cc:

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


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/runtime/hbase-table.cc
File be/src/runtime/hbase-table.cc:

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!


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/runtime/multi-precision-test.cc
File be/src/runtime/multi-precision-test.cc:

Line 81:   int128_t x = 0;
quiet the initialization warnings


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/runtime/tmp-file-mgr.h
File be/src/runtime/tmp-file-mgr.h:

Line 195:       const TUniqueId& query_id, std::unique_ptr<File>* new_file);
Help prevent memory leaks


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/statestore/statestore.h
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


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/util/bloom-filter-test.cc
File be/src/util/bloom-filter-test.cc:

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


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/util/buffer-builder.h
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


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/util/minidump.cc
File be/src/util/minidump.cc:

Line 44
There is also a std::error_code


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/util/parquet-reader.cc
File be/src/util/parquet-reader.cc:

Line 54
Already #included


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/util/pprof-path-handlers.cc
File be/src/util/pprof-path-handlers.cc:

Line 97
Not standard comformant, apparently


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/util/progress-updater.cc
File be/src/util/progress-updater.cc:

PS1, Line 55: 
Not an escape sequence


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/util/rle-test.cc
File be/src/util/rle-test.cc:

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


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/util/webserver.cc
File be/src/util/webserver.cc:

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


http://gerrit.cloudera.org:8080/#/c/4758/1/bin/bootstrap_toolchain.py
File bin/bootstrap_toolchain.py:

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


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

Mime
View raw message