Matthew Jacobs has posted comments on this change. Change subject: IMPALA-3924: Ubuntu16 support ......................................................................
Patch Set 4: (4 comments) Thanks, I think it makes sense. Just a few small cleanup things while you're touching these headers. As a reference: https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes http://gerrit.cloudera.org:8080/#/c/3800/4/be/src/exprs/cast-functions-ir.cc File be/src/exprs/cast-functions-ir.cc: Line 18: #include <cmath> please put this above boost http://gerrit.cloudera.org:8080/#/c/3800/4/be/src/runtime/raw-value-ir.cc File be/src/runtime/raw-value-ir.cc: PS4, Line 15: #include "runtime/raw-value.inline.h" : : #include "runtime/decimal-value.inline.h" This seems weird to me. Can you change this to the following and make sure it compiles, or maybe someone else knows why we're not pulling in the actual header directly. #include "runtime/raw-value.h" http://gerrit.cloudera.org:8080/#/c/3800/4/be/src/runtime/raw-value.inline.h File be/src/runtime/raw-value.inline.h: PS4, Line 22: #include <cmath> Put this above the boost header to be safe http://gerrit.cloudera.org:8080/#/c/3800/4/be/src/util/metrics-test.cc File be/src/util/metrics-test.cc: PS4, Line 14: : #include "util/metrics.h" : #include "util/collection-metrics.h" : #include "util/memory-metrics.h" Can you clean this up while you're here, i.e. move these down with the other local includes? -- To view, visit http://gerrit.cloudera.org:8080/3800 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
