Summary: | Add a script that checks for unnecessary #includes in header files | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dumitru Daniliuc <dumi> | ||||
Component: | Tools / Tests | Assignee: | Dumitru Daniliuc <dumi> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, eric, krit | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Dumitru Daniliuc
2010-07-08 13:07:49 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 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.
> 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. |