Bug 41894 - Add a script that checks for unnecessary #includes in header files
Summary: Add a script that checks for unnecessary #includes in header files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dumitru Daniliuc
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-08 13:07 PDT by Dumitru Daniliuc
Modified: 2010-07-09 01:37 PDT (History)
3 users (show)

See Also:


Attachments
patch (5.12 KB, patch)
2010-07-08 15:40 PDT, Dumitru Daniliuc
darin: review+
dumi: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dumitru Daniliuc 2010-07-08 13:07:49 PDT
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.
Comment 1 Dumitru Daniliuc 2010-07-08 15:40:20 PDT
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. :)
Comment 2 Darin Adler 2010-07-08 23:40:39 PDT
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.
Comment 3 Dumitru Daniliuc 2010-07-08 23:55:38 PDT
> 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.
Comment 4 Dumitru Daniliuc 2010-07-09 01:37:15 PDT
landed: r62917.