impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jim Apple (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3676,4321: Use clang as a static analysis tool
Date Wed, 19 Oct 2016 23:28:44 GMT
Jim Apple has posted comments on this change.

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

Patch Set 3:

File be/CMakeLists.txt:

PS2, Line 104: correct
> 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 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.
File be/src/benchmarks/

Line 167: #include "util/cpu-info.h"
> I think you can just remove the include and it will be linked the normal wa
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.
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
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.
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
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
File be/src/exprs/

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.
File be/src/exprs/

PS2, Line 176: static
> I don't think we want static scope since it's a local variable.
Why not?
File be/src/runtime/buffered-tuple-stream.h:

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

Line 456: 
> Need blank line before.
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
File be/src/runtime/string-buffer.h:

Line 27: namespace impala {
> How about we just use strings::Substitute below?
File be/src/util/aligned-new.h:

Line 25: // Objects that must be allocated at alignment greater than that promised by the
> 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
File be/src/util/

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.
File be/src/util/buffer-builder.h:

Line 33:   BufferBuilder(char* dst_buffer, int dst_len)
> Fix the whitespace here?
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?
File be/src/util/

Line 425:   // returns a limited set of strings and all members of that set are safe.
> I think this needs a comment
File bin/

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
File bin/

PS1, Line 26: quite
> quite

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I4ed168488cb30ddeccd0087f3840541d858f9c06
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message