You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Jim Apple (Code Review)" <ge...@cloudera.org> on 2016/10/19 20:13:04 UTC
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Jim Apple has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4758
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
IMPALA-3676: 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.
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, 607 insertions(+), 362 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/4758/1
--
To view, visit http://gerrit.cloudera.org:8080/4758
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4ed168488cb30ddeccd0087f3840541d858f9c06
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4758
to look at the new patch set (#10).
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
IMPALA-3676: 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 exlcudes some checks which
are part of our current style but which would be too laborious to fix
over the entire codebase, like using nullptr rather than NULL.
This patch also fixes a number of small bugs found by clang-tidy.
Finally, this patch adds the class AlignedNew, the purpose of which is
to provide correct alignment on heap-allocated data. The global new
operator only guarantees 16-byte alignment. A class that includes a
member variable that must be aligned on a k-byte boundary for k>16 can
inherit from AlignedNew<k> to ensure correct alignment on the heap,
quieting clang's -Wover-aligned warning. (Static and stack allocation
are required by the standard to respect the alignment of the type and
its member variables, so no extra code is needed for allocation in
those places.)
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/compiler-util.h
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.h
M be/src/util/thread-pool.h
M be/src/util/webserver.cc
M bin/make_impala.sh
A bin/run_clang_tidy.sh
M buildall.sh
R cmake_modules/clang_toolchain.cmake
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
117 files changed, 565 insertions(+), 334 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/4758/10
--
To view, visit http://gerrit.cloudera.org:8080/4758
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4ed168488cb30ddeccd0087f3840541d858f9c06
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
Patch Set 1:
(6 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}")
> Ah right, I didn't realise the ASAN build got the --gcc-toolchain flag in a
I think that might be just for building the toolchain. I tried changing it and compile_commands.json didn't change.
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 it might manifest as a link-time error if you do anything with the
Done
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
> It doesn't make a difference for correctness, and I don't see any reason we
Done
http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/runtime/coordinator.h
File be/src/runtime/coordinator.h:
> See my other commetn about space savings
I'm interested in your thoughts on which struct reordering s are, in your opinion, worth it
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();
> Missed responding to this one - no callers in the Impala codebase catch the
Done
http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:
Line 335
> I don't think there are enough RuntimeProfile objects to produce a noticabl
I have put these variables back in the logical (but suboptimaly packed) order and added a NOLINT annotation. I think the default should be to pack your structs, but that it's fine not to do so sometimes.
Thoughts?
--
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 <jb...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
Patch Set 6:
(8 comments)
The commit message doesn't really tell the full story here. In particular, you should say something about AlignmentNew and what it is for.
The rearrangement of class members to group by type seems like it sacrifices readability for performance in some strange cases - the statestore is a good example, where there is only ever one statestore and it is not (as far as we know) highly sensitive to its in-memory layout. What criteria did you use to decide that a class' members should be grouped-by-type?
http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/common/init.cc
File be/src/common/init.cc:
PS6, Line 100: [[noreturn]]
can this go on the previous line? It might be easier to read that way (similar to how we do template<> declarations).
http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/exprs/aggregate-functions-ir.cc
File be/src/exprs/aggregate-functions-ir.cc:
PS6, Line 173:
:
Why remove these comments?
PS6, Line 176:
why remove this?
http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/exprs/aggregate-functions.h
File be/src/exprs/aggregate-functions.h:
PS6, Line 182: 10
can you replace that with HLL_PRECISION?
http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
PS6, Line 140: std::
remove std:: in cc files
http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:
PS6, Line 82: CacheLineAligned
why?
PS6, Line 253: boost::mutex exit_flag_lock_;
This is a good example of where logical grouping is broken by grouping-by-type. The lock and the flag are related, but are not next to each other.
http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/util/aligned-new.h
File be/src/util/aligned-new.h:
PS6, Line 27: Objects that must be allocated
What does 'must be allocated' mean - for correctness or performance or both?
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3676,4321: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
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
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4ed168488cb30ddeccd0087f3840541d858f9c06
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
Patch Set 11: Code-Review+2
--
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: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3676,4321: Use clang as a static analysis tool
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 = &AggregateFunctions::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 <jb...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4758
to look at the new patch set (#9).
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
IMPALA-3676: 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 exlcudes some checks which
are part of our current style but which would be too laborious to fix
over the entire codebase, like using nullptr rather than NULL.
This patch also fixes a number of small bugs found by clang-tidy.
Finally, this patch adds the class AlignedNew, the purpose of which is
to provide correct alignment on heap-allocated data. The global new
operator only guarantees 16-byte alignment. A class that includes a
member variable that must be aligned on a k-byte boundary for k>16 can
inherit from AlignedNew<k> to ensure correct alignment on the heap,
quieting clang's -Wover-aligned warning. (Static and stack allocation
are required by the standard to respect the alignment of the type and
its member variables, so no extra code is needed for allocation in
those places.)
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/compiler-util.h
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.h
M be/src/util/thread-pool.h
M be/src/util/webserver.cc
M bin/make_impala.sh
A bin/run_clang_tidy.sh
M buildall.sh
R cmake_modules/clang_toolchain.cmake
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
117 files changed, 570 insertions(+), 335 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/4758/9
--
To view, visit http://gerrit.cloudera.org:8080/4758
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4ed168488cb30ddeccd0087f3840541d858f9c06
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has uploaded a new patch set (#4).
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
IMPALA-3676: 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 exlcudes some checks which
are part of our current style but which would be too laborious to fix
over the entire codebase, like using nullptr rather than NULL.
This patch also fixes a number of small bugs found by clang-tidy.
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.h
M be/src/util/thread-pool.h
M be/src/util/webserver.cc
A bin/run_clang_tidy.sh
M buildall.sh
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
114 files changed, 580 insertions(+), 352 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/4758/4
--
To view, visit http://gerrit.cloudera.org:8080/4758
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4ed168488cb30ddeccd0087f3840541d858f9c06
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
Patch Set 11: Code-Review+1
Forgot to carry Tim's +1 from 23 October
--
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: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
Patch Set 6: Code-Review+1
(1 comment)
Would give a +2 except I think it'd be good to have someone else look at the AlignedNew abstraction - it makes sense to me but I think is somewhat of a new pattern. The rest seems uncontroversial.
http://gerrit.cloudera.org:8080/#/c/4758/6/cmake_modules/clang_toolchain.cmake
File cmake_modules/clang_toolchain.cmake:
Thank you! This makes more sense.
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
Patch Set 12: Verified+1
--
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: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
Patch Set 9:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4758/9/be/src/exec/hdfs-avro-scanner.cc
File be/src/exec/hdfs-avro-scanner.cc:
Line 933: builder->CreateCondBr(ret_val, end_field_block, bail_out);
this still is buggy. but we have IMPALA-4061 to track that, so okay.
http://gerrit.cloudera.org:8080/#/c/4758/9/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) {
the dcheck at line 92 contradicts the need for this if-stmt. is the checker just getting confused, or is there really a path where we can have num_bits > size?
if the checker is confused, can we just disable it here? adding this if-stmt makes it unclear whether num_bits <= size is really an invariant or not.
http://gerrit.cloudera.org:8080/#/c/4758/9/be/src/util/aligned-new.h
File be/src/util/aligned-new.h:
Line 50: };
given that we only really use this for cache alignment, how did you test it?
http://gerrit.cloudera.org:8080/#/c/4758/9/be/src/util/thread-pool.h
File be/src/util/thread-pool.h:
Line 34: class ThreadPool : public CacheLineAligned {
was this aligned before? if not, why are we aligning it now?
--
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3676,4321: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
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 http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4ed168488cb30ddeccd0087f3840541d858f9c06
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
IMPALA-3676: 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 exlcudes some checks which
are part of our current style but which would be too laborious to fix
over the entire codebase, like using nullptr rather than NULL.
This patch also fixes a number of small bugs found by clang-tidy.
Finally, this patch adds the class AlignedNew, the purpose of which is
to provide correct alignment on heap-allocated data. The global new
operator only guarantees 16-byte alignment. A class that includes a
member variable that must be aligned on a k-byte boundary for k>16 can
inherit from AlignedNew<k> to ensure correct alignment on the heap,
quieting clang's -Wover-aligned warning. (Static and stack allocation
are required by the standard to respect the alignment of the type and
its member variables, so no extra code is needed for allocation in
those places.)
Change-Id: I4ed168488cb30ddeccd0087f3840541d858f9c06
Reviewed-on: http://gerrit.cloudera.org:8080/4758
Reviewed-by: Jim Apple <jb...@cloudera.com>
Tested-by: Internal Jenkins
---
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/compiler-util.h
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/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.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.h
M be/src/util/thread-pool.h
M be/src/util/webserver.cc
M bin/make_impala.sh
A bin/run_clang_tidy.sh
M buildall.sh
R cmake_modules/clang_toolchain.cmake
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
114 files changed, 565 insertions(+), 323 deletions(-)
Approvals:
Jim Apple: Looks good to me, approved
Internal Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4758
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I4ed168488cb30ddeccd0087f3840541d858f9c06
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
Patch Set 9: Code-Review+1
PS9 fixes warning from PS8; carry Tim's +1
--
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3676,4321: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
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 <jb...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3676,4321: Use clang as a static analysis tool
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <jb...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
Patch Set 7:
(8 comments)
> (8 comments)
>
> The commit message doesn't really tell the full story here. In
> particular, you should say something about AlignmentNew and what it
> is for.
Done
>
> The rearrangement of class members to group by type seems like it
> sacrifices readability for performance in some strange cases - the
> statestore is a good example, where there is only ever one
> statestore and it is not (as far as we know) highly sensitive to
> its in-memory layout. What criteria did you use to decide that a
> class' members should be grouped-by-type?
I rearranged them all. Tim convinced me to roll some back.
http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/common/init.cc
File be/src/common/init.cc:
PS6, Line 100: [[noreturn]]
> can this go on the previous line? It might be easier to read that way (simi
While it can, clang-format "fixes" it to this right away.
http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/exprs/aggregate-functions-ir.cc
File be/src/exprs/aggregate-functions-ir.cc:
PS6, Line 173:
:
> Why remove these comments?
Accident.
PS6, Line 176:
> why remove this?
Put it back in the definition in the class.
http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/exprs/aggregate-functions.h
File be/src/exprs/aggregate-functions.h:
PS6, Line 182: ec
> can you replace that with HLL_PRECISION?
Done
http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
PS6, Line 140: uniqu
> remove std:: in cc files
Removed here and elsewhere, except for std::move. I see that usually referred to with the prefix by C++ people. Thoughts?
http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:
PS6, Line 82: CacheLineAligned
> why?
subscriber_heartbeat_threadpool_ is a ThreadPool<T>, and ThreadPool<T>::work_queue_ is a BlockingQueue<S>, and BlcokingQueue<S> is marked CACHE_ALIGNED, presumably to avoid false sharing.
I made CacheLineAligned use the C++11 alignas specifier and deleted CACHE_ALIGNED.
PS6, Line 253: boost::mutex exit_flag_lock_;
> This is a good example of where logical grouping is broken by grouping-by-t
rolled back
http://gerrit.cloudera.org:8080/#/c/4758/6/be/src/util/aligned-new.h
File be/src/util/aligned-new.h:
PS6, Line 27: Objects that should be allocate
> What does 'must be allocated' mean - for correctness or performance or both
either - performance for BlockingQueue and correctness for SIMD values when using aligned loads or stores.
--
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has uploaded a new patch set (#5).
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
IMPALA-3676: 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 exlcudes some checks which
are part of our current style but which would be too laborious to fix
over the entire codebase, like using nullptr rather than NULL.
This patch also fixes a number of small bugs found by clang-tidy.
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/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.h
M be/src/util/thread-pool.h
M be/src/util/webserver.cc
M bin/make_impala.sh
A bin/run_clang_tidy.sh
M buildall.sh
R cmake_modules/clang_toolchain.cmake
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
115 files changed, 569 insertions(+), 333 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/4758/5
--
To view, visit http://gerrit.cloudera.org:8080/4758
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4ed168488cb30ddeccd0087f3840541d858f9c06
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
Patch Set 12: Code-Review+2
Carry Dan's +2 on this rebase
--
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: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
Patch Set 7:
> > I rearranged them all. Tim convinced me to roll some back.
>
> That doesn't really answer my question - given the current state of
> the patch, what are the criteria we should use, going forward, to
> decide whether a class should be CacheAligned?
>
> I suggest we reserve CacheAligned / grouping for classes that
> really need it, given the reduction in comprehension. Or am I
> underestimating how big the impact of properly aligning even
> non-performance critical classes will be?
CacheAligned and grouping I view separately.
clang-tidy has a warning about cache alignment that is about correctness - If X must be cache aligned because we wanted it to be so for performance reasons, than clang will warn when NEWing classes that contain that class because we are breaking our own expectations about alignment. The cache alignments I added were only to quiet that warning. I did not set the BlockingQueue to be cache-aligned - that was there when I started.
For grouping, I think you and I are on the same page.
--
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
Patch Set 11:
PS11 was a rebase
--
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: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has uploaded a new patch set (#6).
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
IMPALA-3676: 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 exlcudes some checks which
are part of our current style but which would be too laborious to fix
over the entire codebase, like using nullptr rather than NULL.
This patch also fixes a number of small bugs found by clang-tidy.
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.h
M be/src/util/thread-pool.h
M be/src/util/webserver.cc
M bin/make_impala.sh
A bin/run_clang_tidy.sh
M buildall.sh
R cmake_modules/clang_toolchain.cmake
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
116 files changed, 570 insertions(+), 334 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/4758/6
--
To view, visit http://gerrit.cloudera.org:8080/4758
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4ed168488cb30ddeccd0087f3840541d858f9c06
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4758
to look at the new patch set (#11).
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
IMPALA-3676: 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 exlcudes some checks which
are part of our current style but which would be too laborious to fix
over the entire codebase, like using nullptr rather than NULL.
This patch also fixes a number of small bugs found by clang-tidy.
Finally, this patch adds the class AlignedNew, the purpose of which is
to provide correct alignment on heap-allocated data. The global new
operator only guarantees 16-byte alignment. A class that includes a
member variable that must be aligned on a k-byte boundary for k>16 can
inherit from AlignedNew<k> to ensure correct alignment on the heap,
quieting clang's -Wover-aligned warning. (Static and stack allocation
are required by the standard to respect the alignment of the type and
its member variables, so no extra code is needed for allocation in
those places.)
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/compiler-util.h
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/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.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.h
M be/src/util/thread-pool.h
M be/src/util/webserver.cc
M bin/make_impala.sh
A bin/run_clang_tidy.sh
M buildall.sh
R cmake_modules/clang_toolchain.cmake
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
114 files changed, 565 insertions(+), 323 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/4758/11
--
To view, visit http://gerrit.cloudera.org:8080/4758
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4ed168488cb30ddeccd0087f3840541d858f9c06
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4758
to look at the new patch set (#7).
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
IMPALA-3676: 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 exlcudes some checks which
are part of our current style but which would be too laborious to fix
over the entire codebase, like using nullptr rather than NULL.
This patch also fixes a number of small bugs found by clang-tidy.
Finally, this patch adds the class AlignedNew, the purpose of which is
to provide correct alignment on heap-allocated data. The global new
operator only guarantees 16-byte alignment. A class that includes a
member variable that must be aligned on a k-byte boundary for k>16 can
inherit from AlignedNew<k> to ensure correct alignment on the heap,
quieting clang's -Wover-aligned warning. (Static and stack allocation
are required by the standard to respect the alignment of the type and
its member variables, so no extra code is needed for allocation in
those places.)
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/compiler-util.h
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.h
M be/src/util/thread-pool.h
M be/src/util/webserver.cc
M bin/make_impala.sh
A bin/run_clang_tidy.sh
M buildall.sh
R cmake_modules/clang_toolchain.cmake
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
117 files changed, 568 insertions(+), 334 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/4758/7
--
To view, visit http://gerrit.cloudera.org:8080/4758
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4ed168488cb30ddeccd0087f3840541d858f9c06
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3676,4321: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
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 = &AggregateFunctions::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 <size_t ALIGNMENT>
> 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
You don't think it's worth the space savings?
http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/util/webserver.cc
File be/src/util/webserver.cc:
Line 425: // returns a limited set of strings and all members of that set are safe.
> I think this needs a comment
Done
http://gerrit.cloudera.org:8080/#/c/4758/1/bin/impala-config.sh
File bin/impala-config.sh:
Line 260: export IMPALA_GOOGLETEST_VERSION=20151222
> I feel like we should add the latest release to the toolchain instead of us
Since that's the version that's already in S3, I'd like to make this a followon patch
http://gerrit.cloudera.org:8080/#/c/4758/1/bin/run_clang_tidy.sh
File bin/run_clang_tidy.sh:
PS1, Line 26: quite
> quite
Done
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
Patch Set 6:
> I rearranged them all. Tim convinced me to roll some back.
That doesn't really answer my question - given the current state of the patch, what are the criteria we should use, going forward, to decide whether a class should be CacheAligned?
I suggest we reserve CacheAligned / grouping for classes that really need it, given the reduction in comprehension. Or am I underestimating how big the impact of properly aligning even non-performance critical classes will be?
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <jb...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
Patch Set 8:
PS8 is rebase-only
--
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
Patch Set 5:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/4758/2/be/CMakeLists.txt
File be/CMakeLists.txt:
Line 106: if ("${CMAKE_BUILD_TYPE}" STREQUAL "DEBUG")
> It sets up CMake to use clang as the C++ compiler (see bin/impala_impala.sh
Done
http://gerrit.cloudera.org:8080/#/c/4758/4/be/src/common/status.h
File be/src/common/status.h:
Line 212: return *msg_; // NOLINT: clang-tidy thinks this might deref a nullptr
> I still think we should reword this to make it clear that it's a limitation
Done
http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/runtime/coordinator.h
File be/src/runtime/coordinator.h:
> Either if the multiple instances of the struct are touched on a particularl
WFM
http://gerrit.cloudera.org:8080/#/c/4758/4/be/src/runtime/coordinator.h
File be/src/runtime/coordinator.h:
Line 432: /// safe to concurrently read from filter_routing_table_.
> Can you revert these changes?
Done
http://gerrit.cloudera.org:8080/#/c/4758/1/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:
Line 335
> I'm ok with this but we should revisit if there are a lot of spurious warni
SGTM
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 on
> 2 space indents are the (imperfectly followed) standard in the shell script
Done
http://gerrit.cloudera.org:8080/#/c/4758/4/buildall.sh
File buildall.sh:
Line 256:
> Remove extra blank line.
Done
--
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
Patch Set 9:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4758/9/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) {
> the dcheck at line 92 contradicts the need for this if-stmt. is the checke
undone
http://gerrit.cloudera.org:8080/#/c/4758/9/be/src/util/aligned-new.h
File be/src/util/aligned-new.h:
Line 50: };
> given that we only really use this for cache alignment, how did you test it
I ran core tests. I did not add any unit tests.
http://gerrit.cloudera.org:8080/#/c/4758/9/be/src/util/thread-pool.h
File be/src/util/thread-pool.h:
Line 34: class ThreadPool : public CacheLineAligned {
> was this aligned before? if not, why are we aligning it now?
Clang complained that it should have been aligned to meet the invariant we asked for on BlockingQueue
--
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3676,4321: Use clang as a static analysis tool
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <size_t ALIGNMENT>
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 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 h
I think this needs a comment
http://gerrit.cloudera.org:8080/#/c/4758/1/bin/impala-config.sh
File bin/impala-config.sh:
Line 260: export IMPALA_GOOGLETEST_VERSION=20151222
I feel like we should add the latest release to the toolchain instead of using a random year-old snapshot. https://github.com/google/googletest/releases
http://gerrit.cloudera.org:8080/#/c/4758/1/bin/run_clang_tidy.sh
File bin/run_clang_tidy.sh:
PS1, Line 26: quote
quite
--
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 <jb...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3676: Use clang as a static analysis tool
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3676: Use clang as a static analysis tool
......................................................................
Patch Set 4:
This passes tests with the "core" exploration strategy
--
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 <jb...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No