impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3676,4321: Use clang as a static analysis tool
Date Wed, 19 Oct 2016 21:58:18 GMT
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 <jbapple@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message