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