Alexey Serbin has posted comments on this change. Change subject: WIP: [iwyu] first pass ......................................................................
Patch Set 5: (3 comments) > (3 comments) > > Just did a scan through, mostly looked at fs/ and tserver/ (for > fun). > > One thing comes to mind: this will bitrot very quickly if IWYU > doesn't run precommit (as part of gerrit) to flag bad behavior. How > realistic do you think that is right now? Yes, I agree -- the changes in header files are very common, and it took me a while to keep up with the flow of incoming patches even for this single patch. As for the automating the IWYU checks, I think it's possible to take care of that over the coming weekend (formally, I'm not allowed to spend my time on this since the consistent operations stuff is the higher priority thing right now). I have part of the missing pieces -- the patch to fetch and build IWYU with the llvm suite (need to update to the newer IWYU version which appeared in October). But I need to run the IWYU in many passes with the map files I built and make sure no more suggestions appear. Also, I need to disregard the auto-gen protobuf stuff (will use awk for output parsing, so that sounds easy to do). Also, the codegen test is broken due to some unexpected reason. I need to figure what went wrong with that. But if you are familiar with that part and can guess what's the problem (it could save me some time which I would appreciate a lot), see below for the error: F1128 19:14:14.026911 2025766912 codegen-test.cc:257] Check failed: _s.ok() Bad status: Configuration error: Code generation for module failed. Could not start ExecutionEngine: Interpreter has not been linked in. http://gerrit.cloudera.org:8080/#/c/4738/5/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: Line 38: DECLARE_bool(block_coalesce_close); > I wonder why this is here. I can't see a good reason. Good catch -- it seems we can remove this declaration along with <gflags/gflags_declare.h> header http://gerrit.cloudera.org:8080/#/c/4738/5/src/kudu/tserver/scanners-test.cc File src/kudu/tserver/scanners-test.cc: Line 30: #include <gtest/gtest.h> > Nit: this belongs before the gutil includes. yep -- it seems I did a vim command typo while working with those text blocks. I also missed the <gflags/gflags_declare.h> header. Will fix, thanks. http://gerrit.cloudera.org:8080/#/c/4738/5/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: Line 25: #include <boost/none.hpp> > Nit: belongs in the same grouping as gflags/glog/protobuf. Done -- 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 <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
