On Sat, Aug 3, 2013 at 6:36 AM, Nicholas Nethercote <n.netherc...@gmail.com> wrote: > Gregory suggested that headers aren't something that the build config > group can tackle, and I agree. Modifying #include statements en masse > is much easier if you have some familiarity with the code. You need a > sense of which headers should include which others, and often you have > to move code around. So I encourage people to use IWYU on parts of > the code they are familiar with. (Aryeh did this with editor/ in > https://bugzilla.mozilla.org/show_bug.cgi?id=772807.)
I'm not sure how much it helped, though, as I note in the bug. It didn't appreciably reduce clobber build times. It might have reduced recompile times -- but maybe not, because a lot of the removed headers were included by one of the other included headers anyway. It's very nice that IWYU was able to remove the nsString.h include from nsComposerCommands.h, but probably every .cpp that includes nsComposerCommands.h includes nsString.h somehow anyway, in which case it doesn't really help anything. I should also note that I did once spend a bit of time poking at high-profile headers to see if I could manually remove dependency on other headers somehow, but failed. Things like nsCOMPtr.h already have a pretty minimal set of includes. > SpiderMonkey had ~30 header files that violated this, most of them by > doing something like this: > > #if !defined(jsion_baseline_frame_inl_h__) && defined(JS_ION) > ... > #endif > > I fixed these in https://bugzilla.mozilla.org/show_bug.cgi?id=881579. > Unlike the #include minimization, these don't require domain-specific > expertise and are easy to fix. Did you measure a noticeable performance improvement? I can't imagine that it would take too much time to include a file that's scanned and skipped in its entirety by the preprocessor without invoking the actual compiler, even without this optimization. On Sat, Aug 3, 2013 at 6:59 AM, L. David Baron <dba...@dbaron.org> wrote: > This tool sounds great. I suspect there's even more to be gained > that it can't detect, though, from things that are used, but could > easily be made not used. It has an option to add comments for why headers are included, like #include "nsISupportsImpl.h" // for nsPresContext::Release #include "nsISupportsUtils.h" // for NS_IF_ADDREF #include "nsIURI.h" // for nsIURI #include "nsPresContext.h" // for nsPresContext #include "nscore.h" // for NS_IMETHODIMP, nsresult, etc If you're familiar with the code in the file, you should be able to spot unneeded includes more easily with this info. For instance, NS_IF_ADDREF could probably be removed from nsComposerDocumentCommands.cpp (if it hasn't been already), and then the nsISupportUtils.h include could be removed. nsIURI, most likely not. In practice, I found IWYU to be somewhat frustrating to use because of a few key bugs and omissions (which I don't remember off the top of my head, but filed in their bug tracker, and IIRC they weren't fixed). It also didn't seem willing to look at header files that didn't correspond to source files, which is a lot of the most interesting ones, and it crashed a lot. If it were a bit more polished and reliable, it would be great to run automatically on every checkin, IMO! It will make the changes for you automatically if you give it the right instructions (see exact command in bug 772807 comment 0). The alphabetization and commenting are nice features on their own even without performance improvement. But I didn't find it to be worth it in practice. Which is a pity, because it was a close thing. _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
