Last Comment Bug 902917 - Minimize #include statements some more
: Minimize #include statements some more
Status: RESOLVED FIXED
[include-what-you-use]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla26
Assigned To: Nicholas Nethercote [:njn]
:
:
Mentors:
Depends on: 888768 906626
Blocks: iwyu
  Show dependency treegraph
 
Reported: 2013-08-08 07:26 PDT by Nicholas Nethercote [:njn]
Modified: 2013-08-19 03:23 PDT (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
blah (8.18 KB, patch)
2013-08-08 07:27 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah2 (6.42 KB, patch)
2013-08-08 07:28 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (9.60 KB, patch)
2013-08-08 07:33 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (6.21 KB, patch)
2013-08-08 07:34 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (4.46 KB, patch)
2013-08-08 07:35 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (3.22 KB, patch)
2013-08-08 07:35 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (4.28 KB, patch)
2013-08-08 07:36 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (3.64 KB, patch)
2013-08-08 07:36 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (4.40 KB, patch)
2013-08-08 07:37 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (5.60 KB, patch)
2013-08-08 07:42 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (6.08 KB, patch)
2013-08-08 07:42 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (9.91 KB, patch)
2013-08-08 07:43 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (4.40 KB, patch)
2013-08-08 07:46 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (5.77 KB, patch)
2013-08-08 07:47 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (1.88 KB, patch)
2013-08-08 07:48 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (3.00 KB, patch)
2013-08-08 07:49 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (4.01 KB, patch)
2013-08-08 07:50 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (4.55 KB, patch)
2013-08-08 07:51 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (3.43 KB, patch)
2013-08-08 07:51 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (3.81 KB, patch)
2013-08-08 07:52 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (3.32 KB, patch)
2013-08-08 07:53 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (5.50 KB, patch)
2013-08-08 07:54 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (3.93 KB, patch)
2013-08-08 07:55 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (3.52 KB, patch)
2013-08-08 07:55 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (12.92 KB, patch)
2013-08-08 07:57 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (6.23 KB, patch)
2013-08-08 07:57 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (6.49 KB, patch)
2013-08-08 07:58 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (3.75 KB, patch)
2013-08-08 07:59 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (5.25 KB, patch)
2013-08-08 08:00 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (4.32 KB, patch)
2013-08-08 08:01 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (5.18 KB, patch)
2013-08-08 08:01 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (7.38 KB, patch)
2013-08-08 08:02 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (3.39 KB, patch)
2013-08-08 08:03 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
blah (2.56 KB, patch)
2013-08-08 08:04 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2013-08-08 07:26:05 PDT
Till, I'm going to post a whole lot of small patches.  I've done it that way because I think it makes the reviewing easier.  However, I'll probably fold them up into a few larger patches before landing, because that makes rebasing easier.  I'll warn you in advance that the patch descriptions won't be pretty.

These patches require a certain degree of faith -- often #includes are removed from one place, only to be added in other places, and it's not always easy to understand why (which is why having an automated tool is so helpful).  Generally when that happens, either (a) the removed #includes are in .h files and the added #includes are in .cpp files, and/or (b) the added #includes are lower-level than the removed #includes.
Comment 1 Nicholas Nethercote [:njn] 2013-08-08 07:27:55 PDT
Created attachment 787517 [details] [diff] [review]
blah
Comment 2 Nicholas Nethercote [:njn] 2013-08-08 07:28:59 PDT
Created attachment 787518 [details] [diff] [review]
blah2
Comment 3 Nicholas Nethercote [:njn] 2013-08-08 07:33:32 PDT
Created attachment 787524 [details] [diff] [review]
blah
Comment 4 Nicholas Nethercote [:njn] 2013-08-08 07:34:20 PDT
Created attachment 787526 [details] [diff] [review]
blah
Comment 5 Nicholas Nethercote [:njn] 2013-08-08 07:35:01 PDT
Created attachment 787528 [details] [diff] [review]
blah
Comment 6 Nicholas Nethercote [:njn] 2013-08-08 07:35:30 PDT
Created attachment 787529 [details] [diff] [review]
blah
Comment 7 Nicholas Nethercote [:njn] 2013-08-08 07:36:15 PDT
Created attachment 787530 [details] [diff] [review]
blah
Comment 8 Nicholas Nethercote [:njn] 2013-08-08 07:36:50 PDT
Created attachment 787532 [details] [diff] [review]
blah
Comment 9 Nicholas Nethercote [:njn] 2013-08-08 07:37:23 PDT
Created attachment 787534 [details] [diff] [review]
blah
Comment 10 Nicholas Nethercote [:njn] 2013-08-08 07:42:09 PDT
Created attachment 787537 [details] [diff] [review]
blah
Comment 11 Nicholas Nethercote [:njn] 2013-08-08 07:42:57 PDT
Created attachment 787539 [details] [diff] [review]
blah
Comment 12 Nicholas Nethercote [:njn] 2013-08-08 07:43:58 PDT
Created attachment 787541 [details] [diff] [review]
blah
Comment 13 Nicholas Nethercote [:njn] 2013-08-08 07:46:28 PDT
Created attachment 787546 [details] [diff] [review]
blah
Comment 14 Nicholas Nethercote [:njn] 2013-08-08 07:47:09 PDT
Created attachment 787548 [details] [diff] [review]
blah
Comment 15 Nicholas Nethercote [:njn] 2013-08-08 07:48:26 PDT
Created attachment 787549 [details] [diff] [review]
blah
Comment 16 Nicholas Nethercote [:njn] 2013-08-08 07:49:19 PDT
Created attachment 787550 [details] [diff] [review]
blah
Comment 17 Nicholas Nethercote [:njn] 2013-08-08 07:50:32 PDT
Created attachment 787551 [details] [diff] [review]
blah
Comment 18 Nicholas Nethercote [:njn] 2013-08-08 07:51:04 PDT
Created attachment 787552 [details] [diff] [review]
blah
Comment 19 Nicholas Nethercote [:njn] 2013-08-08 07:51:43 PDT
Created attachment 787553 [details] [diff] [review]
blah
Comment 20 Nicholas Nethercote [:njn] 2013-08-08 07:52:19 PDT
Created attachment 787555 [details] [diff] [review]
blah
Comment 21 Nicholas Nethercote [:njn] 2013-08-08 07:53:27 PDT
Created attachment 787556 [details] [diff] [review]
blah
Comment 22 Nicholas Nethercote [:njn] 2013-08-08 07:54:10 PDT
Created attachment 787557 [details] [diff] [review]
blah
Comment 23 Nicholas Nethercote [:njn] 2013-08-08 07:55:03 PDT
Created attachment 787558 [details] [diff] [review]
blah
Comment 24 Nicholas Nethercote [:njn] 2013-08-08 07:55:55 PDT
Created attachment 787559 [details] [diff] [review]
blah
Comment 25 Nicholas Nethercote [:njn] 2013-08-08 07:57:01 PDT
Created attachment 787561 [details] [diff] [review]
blah
Comment 26 Nicholas Nethercote [:njn] 2013-08-08 07:57:48 PDT
Created attachment 787562 [details] [diff] [review]
blah
Comment 27 Nicholas Nethercote [:njn] 2013-08-08 07:58:55 PDT
Created attachment 787564 [details] [diff] [review]
blah
Comment 28 Nicholas Nethercote [:njn] 2013-08-08 07:59:29 PDT
Created attachment 787565 [details] [diff] [review]
blah
Comment 29 Nicholas Nethercote [:njn] 2013-08-08 08:00:27 PDT
Created attachment 787566 [details] [diff] [review]
blah
Comment 30 Nicholas Nethercote [:njn] 2013-08-08 08:01:06 PDT
Created attachment 787567 [details] [diff] [review]
blah
Comment 31 Nicholas Nethercote [:njn] 2013-08-08 08:01:39 PDT
Created attachment 787568 [details] [diff] [review]
blah
Comment 32 Nicholas Nethercote [:njn] 2013-08-08 08:02:32 PDT
Created attachment 787569 [details] [diff] [review]
blah

jsstrinlines.h is only #included by jsstr.cpp now, so this patch just inlines
it.
Comment 33 Nicholas Nethercote [:njn] 2013-08-08 08:03:22 PDT
Created attachment 787570 [details] [diff] [review]
blah
Comment 34 Nicholas Nethercote [:njn] 2013-08-08 08:04:51 PDT
Created attachment 787572 [details] [diff] [review]
blah
Comment 35 Nicholas Nethercote [:njn] 2013-08-08 08:05:44 PDT
I haven't even tried to minimize #includes of ion headers yet, but this is enough for one bug, I think.
Comment 36 Till Schneidereit [:till] 2013-08-08 08:39:35 PDT
Comment on attachment 787517 [details] [diff] [review]
blah

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

r=me
Comment 37 Till Schneidereit [:till] 2013-08-08 08:40:55 PDT
Comment on attachment 787518 [details] [diff] [review]
blah2

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

r=me
Comment 38 Till Schneidereit [:till] 2013-08-08 08:52:20 PDT
Comment on attachment 787524 [details] [diff] [review]
blah

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

nice, nice

::: js/src/jsnum.h
@@ +6,5 @@
>  
>  #ifndef jsnum_h
>  #define jsnum_h
>  
>  #include "mozilla/FloatingPoint.h"

Shouldn't the ordering have been fixed by another script?
Comment 39 Till Schneidereit [:till] 2013-08-08 08:55:23 PDT
Comment on attachment 787526 [details] [diff] [review]
blah

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

I like it!
Comment 40 Till Schneidereit [:till] 2013-08-08 09:05:51 PDT
Comment on attachment 787529 [details] [diff] [review]
blah

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

yes
Comment 41 Till Schneidereit [:till] 2013-08-08 09:06:51 PDT
Comment on attachment 787528 [details] [diff] [review]
blah

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

nice
Comment 42 Till Schneidereit [:till] 2013-08-08 09:11:31 PDT
Comment on attachment 787530 [details] [diff] [review]
blah

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

makes sense
Comment 43 Till Schneidereit [:till] 2013-08-08 09:16:44 PDT
Comment on attachment 787534 [details] [diff] [review]
blah

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

indeed
Comment 44 Till Schneidereit [:till] 2013-08-08 09:16:44 PDT
Comment on attachment 787532 [details] [diff] [review]
blah

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

yup
Comment 45 Till Schneidereit [:till] 2013-08-08 09:16:57 PDT
Comment on attachment 787537 [details] [diff] [review]
blah

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

r=me
Comment 46 Till Schneidereit [:till] 2013-08-08 09:19:39 PDT
Comment on attachment 787539 [details] [diff] [review]
blah

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

totally
Comment 47 Till Schneidereit [:till] 2013-08-08 09:33:11 PDT
Comment on attachment 787541 [details] [diff] [review]
blah

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

neat
Comment 48 Till Schneidereit [:till] 2013-08-08 09:33:38 PDT
Comment on attachment 787546 [details] [diff] [review]
blah

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

right
Comment 49 Till Schneidereit [:till] 2013-08-08 09:34:26 PDT
Comment on attachment 787548 [details] [diff] [review]
blah

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

good
Comment 50 Till Schneidereit [:till] 2013-08-08 09:35:20 PDT
Comment on attachment 787549 [details] [diff] [review]
blah

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

short, but nice
Comment 51 Till Schneidereit [:till] 2013-08-08 09:36:49 PDT
Comment on attachment 787550 [details] [diff] [review]
blah

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

Less jsfun, what's not to like?
Comment 52 Till Schneidereit [:till] 2013-08-08 09:38:58 PDT
Comment on attachment 787551 [details] [diff] [review]
blah

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

yes
Comment 53 Till Schneidereit [:till] 2013-08-08 09:39:48 PDT
Comment on attachment 787552 [details] [diff] [review]
blah

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

r=me
Comment 54 Till Schneidereit [:till] 2013-08-08 10:14:52 PDT
Comment on attachment 787553 [details] [diff] [review]
blah

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

Lovely
Comment 55 Till Schneidereit [:till] 2013-08-08 10:15:26 PDT
Comment on attachment 787555 [details] [diff] [review]
blah

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

splendid
Comment 56 Till Schneidereit [:till] 2013-08-08 10:16:09 PDT
Comment on attachment 787556 [details] [diff] [review]
blah

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

agreed
Comment 57 Till Schneidereit [:till] 2013-08-08 10:17:57 PDT
Comment on attachment 787557 [details] [diff] [review]
blah

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

yup
Comment 58 Till Schneidereit [:till] 2013-08-08 10:23:11 PDT
Comment on attachment 787558 [details] [diff] [review]
blah

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

likable
Comment 59 Till Schneidereit [:till] 2013-08-08 10:24:35 PDT
Comment on attachment 787559 [details] [diff] [review]
blah

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

So JSON has no Values, I knew it!
Comment 60 Till Schneidereit [:till] 2013-08-08 10:29:37 PDT
Comment on attachment 787561 [details] [diff] [review]
blah

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

Can't say I liked reviewing this. The outcome's nice, though.
Comment 61 Till Schneidereit [:till] 2013-08-08 10:31:51 PDT
Comment on attachment 787562 [details] [diff] [review]
blah

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

beautiful
Comment 62 Till Schneidereit [:till] 2013-08-08 10:34:14 PDT
Comment on attachment 787564 [details] [diff] [review]
blah

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

terrific
Comment 63 Till Schneidereit [:till] 2013-08-08 10:34:54 PDT
Comment on attachment 787565 [details] [diff] [review]
blah

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

yes, yes
Comment 64 Till Schneidereit [:till] 2013-08-08 10:36:18 PDT
Comment on attachment 787566 [details] [diff] [review]
blah

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

jsinferinlines, of all headers, should really be able to infer more things. Inline. Nice to see that it can.
Comment 65 Till Schneidereit [:till] 2013-08-08 10:37:42 PDT
Comment on attachment 787567 [details] [diff] [review]
blah

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

very nice
Comment 66 Till Schneidereit [:till] 2013-08-08 10:41:08 PDT
Comment on attachment 787568 [details] [diff] [review]
blah

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

I guess
Comment 67 Till Schneidereit [:till] 2013-08-08 10:41:43 PDT
Comment on attachment 787569 [details] [diff] [review]
blah

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

\o/
Comment 68 Till Schneidereit [:till] 2013-08-08 10:43:21 PDT
Comment on attachment 787570 [details] [diff] [review]
blah

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

Yes, potentially with moving the TraceLogging.h include in ArgumentsObject.cpp

::: js/src/vm/ArgumentsObject.cpp
@@ +3,5 @@
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "TraceLogging.h"

I'm not sure I understand why that's up here.
Comment 69 Till Schneidereit [:till] 2013-08-08 10:44:27 PDT
Comment on attachment 787572 [details] [diff] [review]
blah

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

This I like!

And I like that I'm through!
Comment 70 Nicholas Nethercote [:njn] 2013-08-08 15:42:24 PDT
> ::: js/src/jsnum.h
>
> Shouldn't the ordering have been fixed by another script?

I don't understand.  The ordering in this file looks ok.


> ::: js/src/vm/ArgumentsObject.cpp
> @@ +3,5 @@
> >   * This Source Code Form is subject to the terms of the Mozilla Public
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > +#include "TraceLogging.h"
> 
> I'm not sure I understand why that's up here.

Oh, that shouldn't be there.  Sometimes when testing I want to know if a .h file is self-contained, i.e. it #includes everything it needs to.  So I temporarily #include it at the top of ArgumentsObjects.cpp, because that's the first file that gets compiled.  I just forgot to take this out.  Good catch!

Thanks for such a quick turnaround on this review bomb!  And I gave you plenty of practice finding positive things to say :)
Comment 71 Till Schneidereit [:till] 2013-08-08 15:58:12 PDT
(In reply to Nicholas Nethercote [:njn] from comment #70)
> > ::: js/src/jsnum.h
> >
> > Shouldn't the ordering have been fixed by another script?
> 
> I don't understand.  The ordering in this file looks ok.

Damn, I meant to rescind that comment. I forgot about the mozilla/ stuff going before the rest for a moment.

> Thanks for such a quick turnaround on this review bomb!  And I gave you
> plenty of practice finding positive things to say :)

That you did. :) And I very much mean it: this work mustn't be thankless, because it's needed and awesome.
Comment 72 Nicholas Nethercote [:njn] 2013-08-11 23:09:38 PDT
Landed as a single patch, because anything else would have been a total nightmare to rebase (esp. in the presence of bug 902908):
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9b8ad32c72b
Comment 73 Ryan VanderMeulen [:RyanVM] 2013-08-12 12:42:43 PDT
https://hg.mozilla.org/mozilla-central/rev/b9b8ad32c72b

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