Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201610192328.u9JNSiFY029655@ip-10-146-233-104.ec2.internal>
Date: Wed, 19 Oct 2016 23:28:44 +0000
From: "Jim Apple (Code Review)" <gerrit@cloudera.org>
To: Jim Apple <jbapple@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Tim Armstrong <tarmstrong@cloudera.com>
Reply-To: jbapple@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-3676=2C4321=3A_Use_clang_as_a_static_analysis_tool=0A?=
In-Reply-To: <gerrit.1476907983000.I4ed168488cb30ddeccd0087f3840541d858f9c06@gerrit.cloudera.org>
References: <gerrit.1476907983000.I4ed168488cb30ddeccd0087f3840541d858f9c06@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Wed, 19 Oct 2016 23:28:51 -0000

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