impala-reviews mailing list archives

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

Mime
View raw message