Bug 41894 – Add a script that checks for unnecessary #includes in header files

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dumitru Daniliuc]

We should add a script that checks for unnecessary #includes in header files. Once the script is submitted, we can consider adding it to check-webkit-style, and/or use it to manually clean up header files, and/or add other scripts on top of it that automatically remove the unnecessary #includes.

[Dumitru Daniliuc]

Created attachment 60974 [details]
patch

This is the script I have. I'm sure it still needs lots of work, so I expect lots of comments. :)

[Darin Adler]

Comment on attachment 60974 [details]
patch

Great idea. I think we can land it as is, as long as it's not giving us any bad advice. No real need to carefully review it.

Does this replace the existing find-extra-includes script already in the WebKitTools/Scripts directory, or does it have a different purpose?

r=me on landing this, but please remove find-extra-includes if this replaces it, or don’t land this if find-extra-includes already does a better job.

[Dumitru Daniliuc]

> I think we can land it as is, as long as it's not giving us any bad advice.

technically, it's possible to get some false positives. for example, if the header file has implementations that look like this:

#include "A.h"

class B {

void doSomething()
{
    getInstanceOfClassA()->foo();
}

};

in this case, the script will say that it's not necessary to include A.h, which is not true. but i think these cases are quite rare (and maybe the implementation should be moved to the .cpp file?). also, we could probably add per-file whitelists, if this really becomes a problem.

> Does this replace the existing find-extra-includes script already in the WebKitTools/Scripts directory, or does it have a different purpose?

i'm looking at find-extra-includes, and if i understand the code correctly, there are 2 differences:
1. find-extra-includes works with any file; check-header-includes is supposed to work with header files only.
2. find-extra-includes reports only the includes that are not used at all; check-header-includes should also report the includes that could be replaced with forward declarations.

> r=me on landing this, but please remove find-extra-includes if this replaces it, or don’t land this if find-extra-includes already does a better job.

i'm going to land the script and use it to start cleaning up header files based on its output. if we need to improve it in any way, i'm happy to patch it as necessary.

[Dumitru Daniliuc]

landed: r62917.