Last Comment Bug 772807 - Try "include-what-you-use" on editor/
: Try "include-what-you-use" on editor/
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla16
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on:
Blocks: iwyu
  Show dependency treegraph
 
Reported: 2012-07-11 05:36 PDT by :Aryeh Gregor (away until August 15)
Modified: 2013-08-11 03:18 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (185.83 KB, patch)
2012-07-11 05:44 PDT, :Aryeh Gregor (away until August 15)
no flags Details | Diff | Splinter Review
Patch v2 (187.79 KB, patch)
2012-07-13 06:53 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review

Description :Aryeh Gregor (away until August 15) 2012-07-11 05:36:38 PDT
See bug 634839.  In theory, it might cut down on compile times.  In practice, it seems buggy and it didn't create a big improvement for me.  It got down compilation of editor/ on a hot cache with ccache cleared from maybe 26s to 25s.  On the other hand, it's possible that it will reduce incremental compile times by removing unnecessary dependencies.  And it alphabetizes #includes, which is a nice bonus!  And adds comments saying what the files are used for.

For a lot of files it only adds includes, for things that are otherwise only indirectly included.  But for instance, in nsEditor.h it converts a lot of includes to forward declarations -- it figured out that things like nsIContent.h didn't have to be included.  It got nsHTMLCSSUtils.h down to four includes -- nsCOMPtr, nsTArray, prtypes, and nscore.

Sometimes I don't know what it thinks it's doing -- like including nsAString.h, nsString.h, and nsStringFwd.h in the same file.  But that doesn't hurt anything, I guess.

I had to patiently teach it not to include stuff like string-template-def-char.h, but I think I mostly succeeded.

The command I'm using is this, having gotten SVN checkouts as described in bug 634839:

find editor -type f -exec touch {} + && make -k -j16 -C objdir-ff-release/editor/libeditor/base CXX=/tmp/llvm/Release+Asserts/bin/include-what-you-use 2>&1 | tee /tmp/iwyu.out | python /tmp/llvm/tools/clang/tools/include-what-you-use/fix_includes.py --nosafe_headers --comments

I go directory-by-directory because it tends to die.  I have to do manual fixups after that, mostly due to <http://code.google.com/p/include-what-you-use/issues/detail?id=74>.

So in conclusion, this was interesting and doesn't hurt anything, but in retrospect, probably was not worth the time.  :/
Comment 1 :Aryeh Gregor (away until August 15) 2012-07-11 05:44:35 PDT
Created attachment 641021 [details] [diff] [review]
Patch

Try: https://tbpl.mozilla.org/?tree=Try&rev=84495791f484
Comment 2 :Aryeh Gregor (away until August 15) 2012-07-11 05:47:20 PDT
I should add -- I add a bunch of iwyu pragma comments in various files outside of editor/.  Let me know if you think I should ask the owners of those modules for that.  Also, I did look over the include changes for attempts to include private headers instead of public ones, but I might have missed some.
Comment 3 :Ehsan Akhgari 2012-07-11 15:16:12 PDT
Comment on attachment 641021 [details] [diff] [review]
Patch

Review of attachment 641021 [details] [diff] [review]:
-----------------------------------------------------------------

The build failed on Windows.  Can you please fix that before I review this?  The patch is sort of painful to review... ;-)
Comment 4 :Ehsan Akhgari 2012-07-11 21:43:00 PDT
Oh, and thanks a lot for taking this on!  I've been meaning to play with IWYU for such a long time, and you finally beat me to it.  :-)
Comment 5 :Aryeh Gregor (away until August 15) 2012-07-13 06:53:18 PDT
Created attachment 641855 [details] [diff] [review]
Patch v2

New try, compiled successfully on all platforms: https://tbpl.mozilla.org/?tree=Try&rev=142c891a968b
Comment 6 :Ehsan Akhgari 2012-07-13 08:30:06 PDT
Comment on attachment 641855 [details] [diff] [review]
Patch v2

Review of attachment 641855 [details] [diff] [review]:
-----------------------------------------------------------------

Great!  I'll land this right now, since this very easily gets bitrotten!
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-07-13 20:23:14 PDT
https://hg.mozilla.org/mozilla-central/rev/4e24278d2273
Comment 9 :Aryeh Gregor (away until August 15) 2012-07-14 22:19:56 PDT
Thanks!

Note You need to log in before you can comment on or make changes to this bug.