Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15248 )
Change subject: IMPALA-9373: more tactical IWYU fixes ...................................................................... Patch Set 8: (3 comments) This makes sense to me. Thanks for putting this together! I have a couple minor nits, but it would be good to get this in. http://gerrit.cloudera.org:8080/#/c/15248/8/be/src/benchmarks/overflow-benchmark.cc File be/src/benchmarks/overflow-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/15248/8/be/src/benchmarks/overflow-benchmark.cc@27 PS8, Line 27: #include "util/decimal-util.h" Nit: Not just this file: If we are going true IWYU, then we would also want util/decimal-constants.h wherever we use these constants. It doesn't matter from a compilation perspective, but if we end up doing IWYU as a precommit, then we may end up needing it. http://gerrit.cloudera.org:8080/#/c/15248/8/be/src/runtime/sorter.h File be/src/runtime/sorter.h: http://gerrit.cloudera.org:8080/#/c/15248/8/be/src/runtime/sorter.h@30 PS8, Line 30: class RuntimeProfile; Nit: I think this is no longer needed. http://gerrit.cloudera.org:8080/#/c/15248/8/be/src/util/bit-util-test.cc File be/src/util/bit-util-test.cc: http://gerrit.cloudera.org:8080/#/c/15248/8/be/src/util/bit-util-test.cc@39 PS8, Line 39: ArithmeticUtil Nit: From an IWYU perspective, this would imply we need util/arithmetic-util.h. -- To view, visit http://gerrit.cloudera.org:8080/15248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8de71866bdf3211e53560d9bfe930e7657c4d7f1 Gerrit-Change-Number: 15248 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Comment-Date: Tue, 24 Mar 2020 01:16:43 +0000 Gerrit-HasComments: Yes
