Alexey Serbin has posted comments on this change. Change subject: WIP: [iwyu] first pass ......................................................................
Patch Set 5: (4 comments) > (5 comments) > > Only got through about 20% of the files, but it looks like there > are some cases where headers are getting included unnecessarily (I > noticed a few cases of glog/logging.h and int128.h). Is this down > to peculiarities with IWYU, or misconfiguration, or something else? Thank you for the review! The IWYU tool in default configuration does not honor transitive inclusion. That's the reason we see so many of them added. I've chosen the non-transitive configuration because I was actively tossing the includes back and forth -- it would be harder working in transitive case here (i.e. more unexpected breakages while in progress). It has stabilized now and I'm thinking about moving into the transitive mode once cleaning this up a bit and having IWYU mappings ready. As for that int128.h, I responded for the particular case in-line. http://gerrit.cloudera.org:8080/#/c/4738/5/src/kudu/benchmarks/tpch/rpc_line_item_dao.h File src/kudu/benchmarks/tpch/rpc_line_item_dao.h: Line 24: #include <boost/function.hpp> // IWYU pragma: keep > Is this due to a bug in IWYU? Could boost/function_fwd.hpp help? That's more about the IWYU's nature -- it wants you to include the very leaf file that contains the necessary definitions ('leaf' in the terms of the header include chain). E.g., instead of <boost/function.hpp> it suggests to include #include <boost/function/function_template.hpp> In case of bind.hpp or alike it might be worse. The idea to deal with that: create a set of mapping files which would allow IWYU to include 'interface-like' headers. I'm in progress of building those mappings, etc. If you are interested in details, you could take a look at: https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUMappings.md http://gerrit.cloudera.org:8080/#/c/4738/5/src/kudu/benchmarks/tpch/tpch-schemas.h File src/kudu/benchmarks/tpch/tpch-schemas.h: Line 25: #include <glog/logging.h> // for CHECK_OK to be picked up in status.h > Why not include glog/logging.h in status.h? ok, I will clarify on that. http://gerrit.cloudera.org:8080/#/c/4738/5/src/kudu/cfile/binary_prefix_block.h File src/kudu/cfile/binary_prefix_block.h: Line 26: #include <glog/logging.h> > It looks like glog/logging.h is getting included in a bunch of places, I pr Here it is for DCHECK() http://gerrit.cloudera.org:8080/#/c/4738/5/src/kudu/cfile/cfile_reader.cc File src/kudu/cfile/cfile_reader.cc: Line 50: #include "kudu/gutil/int128.h" > This is included here and a few other files, but from a quick glance it doe Good catch! Sometimes it's for uint128, sometimes it's for Uint128High64, sometimes it's for ostream (sic!) Here it's for ostream :) I'll need to make an additional run to check for that obscure things. Also, IWYU in default configuration does not honor transitive inclusion. That's the reason we see so many of them added. -- To view, visit http://gerrit.cloudera.org:8080/4738 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
