[Impala-ASF-CR] IMPALA-3676,4321: Use clang as a static analysis tool

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

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


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/util/aligned-new.h
File be/src/util/aligned-new.h:

Line 42:   throw std::bad_alloc();
> I wrote it as such to mimic the existing operator new, and I think making i
Missed responding to this one - no callers in the Impala codebase catch these 
exceptions, it will just crash the process in a somewhat cryptic way - It'd be 
better to at least log a message.


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

[Impala-ASF-CR] IMPALA-3676,4321: Use clang as a static analysis tool

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

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


Patch Set 2:

(5 comments)

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}")
> They seem to be set for different reasons - this one for clang-tidy, that o
Ah right, I didn't realise the ASAN build got the --gcc-toolchain flag in a 
different way - it's in:

  cmake_modules/asan_toolchain.cmake

Maybe we should rename asan_toolchain to clang_toolchain and also use that for 
the clang-tidy builds?


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

Line 175
> static const int* HLL_PRECISION_PTR = ::HLL_PRECISION;
I think it might manifest as a link-time error if you do anything with the 
address, unless constexpr does something different with its linkage compared 
with static const 


http://gerrit.cloudera.org:8080/#/c/4758/2/be/src/exprs/slot-ref.cc
File be/src/exprs/slot-ref.cc:

PS2, Line 176: static
> Why not?
It doesn't make a difference for correctness, and I don't see any reason we 
would want the compiler to store it in global memory somewhere.

If it's a regular local variable that's a constant the compiler is free to do 
whatever it wants with it, particularly if you don't take the address of it - 
it can optimise it out, put it in the global data section, whatever is most 
appropriate.


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

Line 335
> You don't think it's worth the space savings?
I don't think there are enough RuntimeProfile objects to produce a noticable 
savings, I think we should prioritise readability (e.g. here own_pool_ seems to 
be logically grouped with pool_). For some other objects that are more numerous 
it might make a bigger difference.


http://gerrit.cloudera.org:8080/#/c/4758/1/bin/impala-config.sh
File bin/impala-config.sh:

Line 260: export IMPALA_GOOGLETEST_VERSION=20151222
> Since that's the version that's already in S3, I'd like to make this a foll
Maybe just wait until the follow-on patch to make the change?  It's nice to use 
official releases where possible.


-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3676,4321: Use clang as a static analysis tool

[Jim Apple (Code Review)]

Jim Apple has uploaded a new patch set (#3).

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

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

This patch adds a script to run clang-tidy over the whole code
base. It is a first step towards running clang-tidy over patches as a
tool to help users spot bugs before code review.

Because of the number of clang-tidy checks, this patch only addresses
some of them. In particular, only checks starting with 'clang' are
considered. Many of them which are flaky or not part of our style are
excluded from the analysis.

This patch also fixes a number of small bugs found by clang-tidy and
upgrades gtest.

Change-Id: I4ed168488cb30ddeccd0087f3840541d858f9c06
---
A .clang-tidy
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/benchmarks/bloom-filter-benchmark.cc
M be/src/benchmarks/in-predicate-benchmark.cc
M be/src/benchmarks/parse-timestamp-benchmark.cc
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/impala-ir.cc
M be/src/codegen/llvm-codegen.cc
M be/src/common/init.cc
M be/src/common/logging.h
M be/src/common/status.h
M be/src/exec/delimited-text-parser.cc
M be/src/exec/delimited-text-parser.h
M be/src/exec/exec-node.h
M be/src/exec/external-data-source-executor.h
M be/src/exec/hash-table-test.cc
M be/src/exec/hbase-scan-node.h
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/hdfs-table-writer.h
M be/src/exec/parquet-column-readers.h
M be/src/exec/partitioned-hash-join-builder-ir.cc
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/scanner-context.inline.h
M be/src/exec/write-stream.h
M be/src/experiments/bit-stream-utils.8byte.inline.h
M be/src/experiments/tuple-splitter-test.cc
M be/src/exprs/aggregate-functions-ir.cc
M be/src/exprs/aggregate-functions.h
M be/src/exprs/compound-predicates.h
M be/src/exprs/expr-test.cc
M be/src/exprs/slot-ref.cc
M be/src/exprs/string-functions-ir.cc
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/authentication.cc
M be/src/rpc/authentication.h
M be/src/rpc/thrift-server.h
M be/src/runtime/buffered-block-mgr.h
M be/src/runtime/buffered-tuple-stream.cc
M be/src/runtime/buffered-tuple-stream.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/data-stream-sender.h
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M be/src/runtime/exec-env.h
M be/src/runtime/free-pool-test.cc
M be/src/runtime/hbase-table.cc
M be/src/runtime/hdfs-fs-cache.h
M be/src/runtime/multi-precision-test.cc
M be/src/runtime/runtime-filter-bank.h
M be/src/runtime/runtime-filter.h
M be/src/runtime/scoped-buffer.h
M be/src/runtime/string-buffer.h
M be/src/runtime/timestamp-parse-util.cc
M be/src/runtime/timestamp-value.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/scheduling/query-schedule.cc
M be/src/scheduling/query-schedule.h
M be/src/scheduling/simple-scheduler-test.cc
M be/src/service/fe-support.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-exec-state.cc
M be/src/service/query-exec-state.h
M be/src/service/query-result-set.cc
M be/src/statestore/failure-detector.h
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M be/src/testutil/test-udas.cc
M be/src/testutil/test-udas.h
M be/src/transport/TSasl.h
M be/src/transport/TSaslClientTransport.h
M be/src/transport/TSaslServerTransport.h
M be/src/transport/TSaslTransport.h
M be/src/udf/uda-test-harness.h
M be/src/udf/uda-test.cc
M be/src/udf/udf-ir.cc
M be/src/udf/udf.cc
M be/src/udf/udf.h
M be/src/udf_samples/uda-sample.cc
A be/src/util/aligned-new.h
M be/src/util/benchmark-test.cc
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util-test.cc
M be/src/util/blocking-queue-test.cc
M be/src/util/blocking-queue.h
M be/src/util/bloom-filter-test.cc
M be/src/util/buffer-builder.h
M be/src/util/compress.cc
M be/src/util/decompress.cc
M be/src/util/metrics.h
M be/src/util/minidump.cc
M be/src/util/network-util.cc
M be/src/util/parquet-reader.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/pprof-path-handlers.cc
M be/src/util/progress-updater.cc
M be/src/util/rle-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M be/src/util/thread-pool.h
M be/src/util/webserver.cc
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
A bin/run_clang_tidy.sh
M buildall.sh
M cmake_modules/FindGTest.cmake
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
118 files changed, 609 insertions(+), 369 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/4758/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4758
To unsubscribe, visit 

[Impala-ASF-CR] IMPALA-3676,4321: Use clang as a static analysis tool

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

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


Patch Set 3:

(23 comments)

http://gerrit.cloudera.org:8080/#/c/4758/2/be/CMakeLists.txt
File be/CMakeLists.txt:

PS2, Line 104: correct
> correct
Done


Line 106: SET(CXX_FLAGS_TIDY "${CXX_FLAGS_TIDY} --gcc-toolchain=${GCC_ROOT}")
> We already set this in CLANG_BASE_FLAGS below - not sure why it's done sepa
They seem to be set for different reasons - this one for clang-tidy, that one 
for IR. I think it makes more sense to keep them separate.


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

Line 167: #include "util/cpu-info.h"
> I think you can just remove the include and it will be linked the normal wa
Done


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

Line 45
> I take it this is no longer necessary?
#undefing _XOPEN_SOURCE? Yes, it appears to no longer be necessary.


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

Line 35: #ifdef _GLIBCXX_VECTOR
> I feel we shouldn't do this unless we actually intend to get libcpp working
Done


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

PS2, Line 212: with DCHECKS off, this might deref a nullptr
> We shouldn't actually be dereferencing a NULL pointer - is it just that cla
Kind of.  I mean, we expect that it isn't null, but I don't think we guarantee 
it.


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

Line 169: 
> Was this just cleanup or the result of a clang-tidy warning?
A warning about wasted padding


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:

> How about we just delete this? Probably not worth maintaining it.
I'd like to do that in a followup patch


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

Line 175
> I think we still want to have the declarations up the top of the .cc file s
static const int* HLL_PRECISION_PTR = ::HLL_PRECISION;

compiles for me.


http://gerrit.cloudera.org:8080/#/c/4758/2/be/src/exprs/slot-ref.cc
File be/src/exprs/slot-ref.cc:

PS2, Line 176: static
> I don't think we want static scope since it's a local variable.
Why not?


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/runtime/buffered-tuple-stream.h
File be/src/runtime/buffered-tuple-stream.h:

Line 450:   bool use_small_buffers_;
> Can you reorder so that we have:
Done


Line 456: 
> Need blank line before.
Done


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

> I don't think we should reorder members here since there was some logical g
See my other commetn about space savings


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

Line 27: namespace impala {
> How about we just use strings::Substitute below?
Done


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/util/aligned-new.h
File be/src/util/aligned-new.h:

Line 25: // Objects that must be allocated at alignment greater than that 
promised by the global
> This doesn't help if the class is embedded in another classes, right? Would
Can you elaborate? Do you mean

Foo : CacheLineAligned;
Bar { Foo f; }
// Bar is not cache-line aligned


PS1, Line 27: template 
> Do classes that inherit from this also need to set __attribute__((aligned ?
I don't think they do; I believe the standard requires that stack or static 
variables will be as aligned at least as much as their members.


Line 42:   throw std::bad_alloc();
> We don't catch this exception so it would be better to do a LOG(FATAL).
I wrote it as such to mimic the existing operator new, and I think making it 
different will be surprising behavior to callers who now have to expect that 
new may fail in one of two distinct ways.


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

Line 70: class MultiThreadTest { // NOLINT: members are not arranged for 
minimal padding
> I think we should just remove this warning if it's insisting on us packing 
It can lead to big space savings; that seems worth it to me.


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/util/buffer-builder.h
File be/src/util/buffer-builder.h:

Line 33:   BufferBuilder(char* dst_buffer, int dst_len)
> Fix the whitespace here?
Done


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

Line 335
> Let's not move these around, there was some logic to the grouping

[Impala-ASF-CR] IMPALA-3676,4321: Use clang as a static analysis tool

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

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


Patch Set 1:

(23 comments)

http://gerrit.cloudera.org:8080/#/c/4758/2/be/CMakeLists.txt
File be/CMakeLists.txt:

PS2, Line 104: corredt
correct


Line 106: SET(CXX_FLAGS_TIDY "${CXX_FLAGS_TIDY} --gcc-toolchain=${GCC_ROOT}")
We already set this in CLANG_BASE_FLAGS below - not sure why it's done 
separately, but we should combine it.


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

Line 167: #include "exprs/in-predicate-ir.cc"
I think you can just remove the include and it will be linked the normal way.


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

Line 45
I take it this is no longer necessary?


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
I feel we shouldn't do this unless we actually intend to get libcpp working 
end-to-end and test it automatically (which I don't think we do), because it 
makes it harder to maintain this file and causes confusion about what we 
do/don't support.


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

PS2, Line 212: with DCHECKS off, this might deref a nullptr
We shouldn't actually be dereferencing a NULL pointer - is it just that 
clang-tidy can't infer that the pointer is non-NULL, right?


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

Line 169: 
Was this just cleanup or the result of a clang-tidy warning?


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:

How about we just delete this? Probably not worth maintaining it.


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

Line 175
I think we still want to have the declarations up the top of the .cc file so 
that storage is allocated, e.g. constexpr int AggregateFunctions::HLL_PRECISION;

Otherwise some of the DCHECK macros fail in strange ways. E.g. DCHECK_LE(prec, 
HLL_PRECISION) takes the address of both arguments.


http://gerrit.cloudera.org:8080/#/c/4758/2/be/src/exprs/slot-ref.cc
File be/src/exprs/slot-ref.cc:

PS2, Line 176: static
I don't think we want static scope since it's a local variable.


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/runtime/buffered-tuple-stream.h
File be/src/runtime/buffered-tuple-stream.h:

Line 450:   const bool read_write_;
Can you reorder so that we have:

const members (read_write_/has_nullable_tuple_)
non-const members (closed_/pinned_/delete_on_read_/use_small_buffers_)

It feels a little jumbled


Line 456:   /// If true, this stream has been explicitly pinned by the caller. 
This changes the
Need blank line before.


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

I don't think we should reorder members here since there was some logical 
grouping before.


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

Line 27: using strings::Substitute;
How about we just use strings::Substitute below?


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/util/aligned-new.h
File be/src/util/aligned-new.h:

Line 25: // Objects that must be allocated at alignment greater than that 
promised by the global
This doesn't help if the class is embedded in another classes, right? Would be 
good to call that out.


PS1, Line 27: template 
Do classes that inherit from this also need to set __attribute__((aligned ? 
Would be good to document that this doesn't guarantee anything about alignment 
if the object is allocated in some other way.


Line 42:   throw std::bad_alloc();
We don't catch this exception so it would be better to do a LOG(FATAL).


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

Line 70: class MultiThreadTest { // NOLINT: members are not arranged for 
minimal padding
I think we should just remove this warning if it's insisting on us packing all 
of our structs.


http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/util/buffer-builder.h
File be/src/util/buffer-builder.h:

Line 33:   
Fix the whitespace here?


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

Line 335
Let's not move these around, there was some logic to the grouping


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

[Impala-ASF-CR] IMPALA-3676,4321: Use clang as a static analysis tool

[Jim Apple (Code Review)]

Jim Apple has uploaded a new patch set (#2).

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

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

This patch adds a script to run clang-tidy over the whole code
base. It is a first step towards running clang-tidy over patches as a
tool to help users spot bugs before code review.

Because of the number of clang-tidy checks, this patch only addresses
some of them. In particular, only checks starting with 'clang' are
considered. Many of them which are flaky or not part of our style are
excluded from the analysis.

This patch also fixes a number of small bugs found by clang-tidy and
upgrades gtest.

Change-Id: I4ed168488cb30ddeccd0087f3840541d858f9c06
---
A .clang-tidy
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/benchmarks/bloom-filter-benchmark.cc
M be/src/benchmarks/in-predicate-benchmark.cc
M be/src/benchmarks/parse-timestamp-benchmark.cc
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/impala-ir.cc
M be/src/codegen/llvm-codegen.cc
M be/src/common/init.cc
M be/src/common/logging.h
M be/src/common/names.h
M be/src/common/status.h
M be/src/exec/delimited-text-parser.cc
M be/src/exec/delimited-text-parser.h
M be/src/exec/exec-node.h
M be/src/exec/external-data-source-executor.h
M be/src/exec/hash-table-test.cc
M be/src/exec/hbase-scan-node.h
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/hdfs-table-writer.h
M be/src/exec/parquet-column-readers.h
M be/src/exec/partitioned-hash-join-builder-ir.cc
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/scanner-context.inline.h
M be/src/exec/write-stream.h
M be/src/experiments/bit-stream-utils.8byte.inline.h
M be/src/experiments/tuple-splitter-test.cc
M be/src/exprs/aggregate-functions-ir.cc
M be/src/exprs/aggregate-functions.h
M be/src/exprs/compound-predicates.h
M be/src/exprs/expr-test.cc
M be/src/exprs/slot-ref.cc
M be/src/exprs/string-functions-ir.cc
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/authentication.cc
M be/src/rpc/authentication.h
M be/src/rpc/thrift-server.h
M be/src/runtime/buffered-block-mgr.h
M be/src/runtime/buffered-tuple-stream.cc
M be/src/runtime/buffered-tuple-stream.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/data-stream-sender.h
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M be/src/runtime/exec-env.h
M be/src/runtime/free-pool-test.cc
M be/src/runtime/hbase-table.cc
M be/src/runtime/hdfs-fs-cache.h
M be/src/runtime/multi-precision-test.cc
M be/src/runtime/runtime-filter-bank.h
M be/src/runtime/runtime-filter.h
M be/src/runtime/scoped-buffer.h
M be/src/runtime/string-buffer.h
M be/src/runtime/timestamp-parse-util.cc
M be/src/runtime/timestamp-value.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/scheduling/query-schedule.cc
M be/src/scheduling/query-schedule.h
M be/src/scheduling/simple-scheduler-test.cc
M be/src/service/fe-support.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-exec-state.cc
M be/src/service/query-exec-state.h
M be/src/service/query-result-set.cc
M be/src/statestore/failure-detector.h
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M be/src/testutil/test-udas.cc
M be/src/testutil/test-udas.h
M be/src/transport/TSasl.h
M be/src/transport/TSaslClientTransport.h
M be/src/transport/TSaslServerTransport.h
M be/src/transport/TSaslTransport.h
M be/src/udf/uda-test-harness.h
M be/src/udf/uda-test.cc
M be/src/udf/udf-ir.cc
M be/src/udf/udf.cc
M be/src/udf/udf.h
M be/src/udf_samples/uda-sample.cc
A be/src/util/aligned-new.h
M be/src/util/benchmark-test.cc
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util-test.cc
M be/src/util/blocking-queue-test.cc
M be/src/util/blocking-queue.h
M be/src/util/bloom-filter-test.cc
M be/src/util/buffer-builder.h
M be/src/util/compress.cc
M be/src/util/decompress.cc
M be/src/util/metrics.h
M be/src/util/minidump.cc
M be/src/util/network-util.cc
M be/src/util/parquet-reader.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/pprof-path-handlers.cc
M be/src/util/progress-updater.cc
M be/src/util/rle-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M be/src/util/thread-pool.h
M be/src/util/webserver.cc
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
A bin/run_clang_tidy.sh
M buildall.sh
M cmake_modules/FindGTest.cmake
118 files changed, 606 insertions(+), 362 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/4758/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4758
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

[Impala-ASF-CR] IMPALA-3676,4321: Use clang as a static analysis tool

[Jim Apple (Code Review)]

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 
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 
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