Include-what-you-use is hard -- clang-include-fixer to rescue?!

[Gabriel Charette]

Include-what-you-use is H A R D and it's a pain to have to update over 80 files not including-what-they-use when you're merely trying to strip an unnecessary include from a common header (e.g. after a refactoring or to break a dependency).

Clang now has clang-include-fixer and given we now support clang on most (all?) of our platforms, we could have an IWYU presubmit? (probably not on by default on upload since it requires compiling a tool but at least on CQ and optionally on upload?)

I'd initially be looking to force IWYU for base:: (those are the most painful ones to fix as they tend to be widespread).

What do you think? Not sure how to make this happen but I can try to find the right expertise if there's buy-in :)

Cheers,

Gab

[Nico Weber]

clang-include-fixer shares a name with clang but is a separate binary and whatnot. As far as I know it also only adds includes and doesn't remove them, so it's not really an IWYU thing.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Nico Weber]

(To be clear: It might make sense to use it, but it requires some investigation before proposing that, it probably won't Just Work.)

[Daniel Cheng]

On Tue, Feb 14, 2017 at 11:34 AM Gabriel Charette <g...@chromium.org> wrote:

Include-what-you-use is H A R D and it's a pain to have to update over 80 files not including-what-they-use when you're merely trying to strip an unnecessary include from a common header (e.g. after a refactoring or to break a dependency).

Clang now has clang-include-fixer and given we now support clang on most (all?) of our platforms, we could have an IWYU presubmit? (probably not on by default on upload since it requires compiling a tool but at least on CQ and optionally on upload?)

While IWYU is definitely a good thing, IWYU itself seemed to have a bunch of issues: it doesn't work well with #ifdef'd code, we needed to manually force IWYU not to remove some headers, etc. See some previous attempts at: see https://bugs.chromium.org/p/chromium/issues/detail?id=259043 and https://codereview.chromium.org/1561693003/. 

 

I'd initially be looking to force IWYU for base:: (those are the most painful ones to fix as they tend to be widespread).

What do you think? Not sure how to make this happen but I can try to find the right expertise if there's buy-in :)

I think a reasonable start would be making it an opt-in tool and fixing issues that appear there; then we can make a better judgement about enforcing things at project scope.

From an implementation perspective, even integrating include-fixer will be tricky: it relies on both a compilation DB and a symbol DB (which needs to be generated; presumably by running another clang tool across the entire codebase), and the results are specific to a given build config.

Daniel

[François Doray]

We could have a simple presubmit to enforce:

A file that contains this outside of comments... -> Needs to include this...

base::ThreadChecker -> base/threading/thread_checker.h

base::SequenceChecker -> base/sequence_checker.h

base::ThreadTaskRunnerHandle -> base/threading/thread_task_runner_handle.h

base::SequencedTaskRunnerHandle -> base/threading/sequenced_task_runner_handle.h

base::MakeUnique -> base/memory/ptr_util.h

base::WrapUnique -> base/memory/ptr_util.h

(It's ok if the include is in the .h file)

Note that I didn't include in the list things that can be forward-declared (e.g. TaskRunner). These simple rules don't solve everything, but they are easy to implement, they can run quickly and would have avoided the need for these CLs.

[Alexei Svitkine]

This is similar to what git cl lint does already for std C++ headers - like std::string and std::vector. So maybe the logic could be added to that check?

However, git cl lint doesn't get run unless someone manually does it on their CL. Maybe we should fix that and make presumit run it?

[Gabriel Charette]

I'm in favor of a simple presubmit in the meantime for the most egregious types for which we can derive simple heuristics (i.e. widely used APIs which can't be fwd-decl'd).

I'm not familiar with git cl lint so I don't have an opinion on whether it should be default (I'd assume the bits that matter are done by git cl format?).

But I wouldn't block this on git cl lint (happy to merge later).

+Francois Pierre Doray : IIRC you have a script to do these checks (i.e. the bits about also checking .cc vs .h) which you used to generate those very CLs. Care to turn it into a presubmit?

Besides not duplicating .h/.cc includes the only other IWYU rule that needs to be covered is not having to re-include types which are merely used in overrides (i.e. which inherited header *has* to include by IWYU transitivity). But this isn't relevant for the types you mentioned as "not fwd-decl'd" also means "intended to be used by this very file, not just passed as a param" :).

[Alexei Svitkine]

git cl format only formats code, whereas git cl lint will suggest to add headers (among other things), for example from a CL I'm working on:

ui/base/resource/data_pack.cc:89:  Add #include <set> for set<>  [build/include_what_you_use] [4]

[Alexei Svitkine]

Here's where those existing things are implemented:

https://cs.chromium.org/chromium/tools/depot_tools/cpplint.py?rcl=f3cac415469335979e16a13273b50239400fd1a6&l=5235

However, looks like that lives in depot_tools so might not be the best place to put Chromium-specific things (since depot_tools is used for other repos than Chromium.)