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