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 <jbap...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbap...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
