The default bug view has changed. See this FAQ.

Minimize #include statements some more

RESOLVED FIXED in mozilla26

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks: 1 bug)

unspecified
mozilla26
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [include-what-you-use])

Attachments

(34 attachments)

8.18 KB, patch
till
: review+
Details | Diff | Splinter Review
6.42 KB, patch
till
: review+
Details | Diff | Splinter Review
9.60 KB, patch
till
: review+
Details | Diff | Splinter Review
6.21 KB, patch
till
: review+
Details | Diff | Splinter Review
4.46 KB, patch
till
: review+
Details | Diff | Splinter Review
3.22 KB, patch
till
: review+
Details | Diff | Splinter Review
4.28 KB, patch
till
: review+
Details | Diff | Splinter Review
3.64 KB, patch
till
: review+
Details | Diff | Splinter Review
4.40 KB, patch
till
: review+
Details | Diff | Splinter Review
5.60 KB, patch
till
: review+
Details | Diff | Splinter Review
6.08 KB, patch
till
: review+
Details | Diff | Splinter Review
9.91 KB, patch
till
: review+
Details | Diff | Splinter Review
4.40 KB, patch
till
: review+
Details | Diff | Splinter Review
5.77 KB, patch
till
: review+
Details | Diff | Splinter Review
1.88 KB, patch
till
: review+
Details | Diff | Splinter Review
3.00 KB, patch
till
: review+
Details | Diff | Splinter Review
4.01 KB, patch
till
: review+
Details | Diff | Splinter Review
4.55 KB, patch
till
: review+
Details | Diff | Splinter Review
3.43 KB, patch
till
: review+
Details | Diff | Splinter Review
3.81 KB, patch
till
: review+
Details | Diff | Splinter Review
3.32 KB, patch
till
: review+
Details | Diff | Splinter Review
5.50 KB, patch
till
: review+
Details | Diff | Splinter Review
3.93 KB, patch
till
: review+
Details | Diff | Splinter Review
3.52 KB, patch
till
: review+
Details | Diff | Splinter Review
12.92 KB, patch
till
: review+
Details | Diff | Splinter Review
6.23 KB, patch
till
: review+
Details | Diff | Splinter Review
6.49 KB, patch
till
: review+
Details | Diff | Splinter Review
3.75 KB, patch
till
: review+
Details | Diff | Splinter Review
5.25 KB, patch
till
: review+
Details | Diff | Splinter Review
4.32 KB, patch
till
: review+
Details | Diff | Splinter Review
5.18 KB, patch
till
: review+
Details | Diff | Splinter Review
7.38 KB, patch
till
: review+
Details | Diff | Splinter Review
3.39 KB, patch
till
: review+
Details | Diff | Splinter Review
2.56 KB, patch
till
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 787517 [details] [diff] [review]
blah
Attachment #787517 - Flags: review?(till)
(Assignee)

Updated

4 years ago
Assignee: general → n.nethercote
(Assignee)

Comment 2

4 years ago
Created attachment 787518 [details] [diff] [review]
blah2
Attachment #787518 - Flags: review?(till)
(Assignee)

Comment 3

4 years ago
Created attachment 787524 [details] [diff] [review]
blah
Attachment #787524 - Flags: review?(till)
(Assignee)

Comment 4

4 years ago
Created attachment 787526 [details] [diff] [review]
blah
Attachment #787526 - Flags: review?(till)
(Assignee)

Comment 5

4 years ago
Created attachment 787528 [details] [diff] [review]
blah
Attachment #787528 - Flags: review?(till)
(Assignee)

Comment 6

4 years ago
Created attachment 787529 [details] [diff] [review]
blah
Attachment #787529 - Flags: review?(till)
(Assignee)

Comment 7

4 years ago
Created attachment 787530 [details] [diff] [review]
blah
Attachment #787530 - Flags: review?(till)
(Assignee)

Comment 8

4 years ago
Created attachment 787532 [details] [diff] [review]
blah
Attachment #787532 - Flags: review?(till)
(Assignee)

Comment 9

4 years ago
Created attachment 787534 [details] [diff] [review]
blah
Attachment #787534 - Flags: review?(till)
(Assignee)

Comment 10

4 years ago
Created attachment 787537 [details] [diff] [review]
blah
Attachment #787537 - Flags: review?(till)
(Assignee)

Comment 11

4 years ago
Created attachment 787539 [details] [diff] [review]
blah
Attachment #787539 - Flags: review?(till)
(Assignee)

Comment 12

4 years ago
Created attachment 787541 [details] [diff] [review]
blah
Attachment #787541 - Flags: review?(till)
(Assignee)

Comment 13

4 years ago
Created attachment 787546 [details] [diff] [review]
blah
Attachment #787546 - Flags: review?(till)
(Assignee)

Comment 14

4 years ago
Created attachment 787548 [details] [diff] [review]
blah
Attachment #787548 - Flags: review?(till)
(Assignee)

Comment 15

4 years ago
Created attachment 787549 [details] [diff] [review]
blah
Attachment #787549 - Flags: review?(till)
(Assignee)

Comment 16

4 years ago
Created attachment 787550 [details] [diff] [review]
blah
Attachment #787550 - Flags: review?(till)
(Assignee)

Comment 17

4 years ago
Created attachment 787551 [details] [diff] [review]
blah
Attachment #787551 - Flags: review?(till)
(Assignee)

Comment 18

4 years ago
Created attachment 787552 [details] [diff] [review]
blah
Attachment #787552 - Flags: review?(till)
(Assignee)

Comment 19

4 years ago
Created attachment 787553 [details] [diff] [review]
blah
Attachment #787553 - Flags: review?(till)
(Assignee)

Comment 20

4 years ago
Created attachment 787555 [details] [diff] [review]
blah
Attachment #787555 - Flags: review?(till)
(Assignee)

Comment 21

4 years ago
Created attachment 787556 [details] [diff] [review]
blah
Attachment #787556 - Flags: review?(till)
(Assignee)

Comment 22

4 years ago
Created attachment 787557 [details] [diff] [review]
blah
Attachment #787557 - Flags: review?(till)
(Assignee)

Comment 23

4 years ago
Created attachment 787558 [details] [diff] [review]
blah
Attachment #787558 - Flags: review?(till)
(Assignee)

Comment 24

4 years ago
Created attachment 787559 [details] [diff] [review]
blah
Attachment #787559 - Flags: review?(till)
(Assignee)

Comment 25

4 years ago
Created attachment 787561 [details] [diff] [review]
blah
Attachment #787561 - Flags: review?(till)
(Assignee)

Comment 26

4 years ago
Created attachment 787562 [details] [diff] [review]
blah
Attachment #787562 - Flags: review?(till)
(Assignee)

Comment 27

4 years ago
Created attachment 787564 [details] [diff] [review]
blah
Attachment #787564 - Flags: review?(till)
(Assignee)

Comment 28

4 years ago
Created attachment 787565 [details] [diff] [review]
blah
Attachment #787565 - Flags: review?(till)
(Assignee)

Comment 29

4 years ago
Created attachment 787566 [details] [diff] [review]
blah
Attachment #787566 - Flags: review?(till)
(Assignee)

Comment 30

4 years ago
Created attachment 787567 [details] [diff] [review]
blah
Attachment #787567 - Flags: review?(till)
(Assignee)

Comment 31

4 years ago
Created attachment 787568 [details] [diff] [review]
blah
Attachment #787568 - Flags: review?(till)
(Assignee)

Comment 32

4 years ago
Created attachment 787569 [details] [diff] [review]
blah

jsstrinlines.h is only #included by jsstr.cpp now, so this patch just inlines
it.
Attachment #787569 - Flags: review?(till)
(Assignee)

Comment 33

4 years ago
Created attachment 787570 [details] [diff] [review]
blah
Attachment #787570 - Flags: review?(till)
(Assignee)

Comment 34

4 years ago
Created attachment 787572 [details] [diff] [review]
blah
Attachment #787572 - Flags: review?(till)
(Assignee)

Comment 35

4 years ago
I haven't even tried to minimize #includes of ion headers yet, but this is enough for one bug, I think.
Comment on attachment 787517 [details] [diff] [review]
blah

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

r=me
Attachment #787517 - Flags: review?(till) → review+
Comment on attachment 787518 [details] [diff] [review]
blah2

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

r=me
Attachment #787518 - Flags: review?(till) → review+
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?
Attachment #787524 - Flags: review?(till) → review+
Comment on attachment 787526 [details] [diff] [review]
blah

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

I like it!
Attachment #787526 - Flags: review?(till) → review+
Comment on attachment 787529 [details] [diff] [review]
blah

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

yes
Attachment #787529 - Flags: review?(till) → review+
Comment on attachment 787528 [details] [diff] [review]
blah

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

nice
Attachment #787528 - Flags: review?(till) → review+
Comment on attachment 787530 [details] [diff] [review]
blah

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

makes sense
Attachment #787530 - Flags: review?(till) → review+
Comment on attachment 787534 [details] [diff] [review]
blah

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

indeed
Attachment #787534 - Flags: review?(till) → review+
Attachment #787532 - Flags: review?(till) → review+
Comment on attachment 787532 [details] [diff] [review]
blah

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

yup
Comment on attachment 787537 [details] [diff] [review]
blah

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

r=me
Attachment #787537 - Flags: review?(till) → review+
Comment on attachment 787539 [details] [diff] [review]
blah

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

totally
Attachment #787539 - Flags: review?(till) → review+
Comment on attachment 787541 [details] [diff] [review]
blah

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

neat
Attachment #787541 - Flags: review?(till) → review+
Comment on attachment 787546 [details] [diff] [review]
blah

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

right
Attachment #787546 - Flags: review?(till) → review+
Comment on attachment 787548 [details] [diff] [review]
blah

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

good
Attachment #787548 - Flags: review?(till) → review+
Comment on attachment 787549 [details] [diff] [review]
blah

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

short, but nice
Attachment #787549 - Flags: review?(till) → review+
Comment on attachment 787550 [details] [diff] [review]
blah

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

Less jsfun, what's not to like?
Attachment #787550 - Flags: review?(till) → review+
Comment on attachment 787551 [details] [diff] [review]
blah

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

yes
Attachment #787551 - Flags: review?(till) → review+
Comment on attachment 787552 [details] [diff] [review]
blah

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

r=me
Attachment #787552 - Flags: review?(till) → review+
Comment on attachment 787553 [details] [diff] [review]
blah

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

Lovely
Attachment #787553 - Flags: review?(till) → review+
Comment on attachment 787555 [details] [diff] [review]
blah

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

splendid
Attachment #787555 - Flags: review?(till) → review+
Comment on attachment 787556 [details] [diff] [review]
blah

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

agreed
Attachment #787556 - Flags: review?(till) → review+
Comment on attachment 787557 [details] [diff] [review]
blah

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

yup
Attachment #787557 - Flags: review?(till) → review+
Comment on attachment 787558 [details] [diff] [review]
blah

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

likable
Attachment #787558 - Flags: review?(till) → review+
Comment on attachment 787559 [details] [diff] [review]
blah

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

So JSON has no Values, I knew it!
Attachment #787559 - Flags: review?(till) → review+
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.
Attachment #787561 - Flags: review?(till) → review+
Comment on attachment 787562 [details] [diff] [review]
blah

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

beautiful
Attachment #787562 - Flags: review?(till) → review+
Comment on attachment 787564 [details] [diff] [review]
blah

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

terrific
Attachment #787564 - Flags: review?(till) → review+
Comment on attachment 787565 [details] [diff] [review]
blah

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

yes, yes
Attachment #787565 - Flags: review?(till) → review+
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.
Attachment #787566 - Flags: review?(till) → review+
Comment on attachment 787567 [details] [diff] [review]
blah

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

very nice
Attachment #787567 - Flags: review?(till) → review+
Comment on attachment 787568 [details] [diff] [review]
blah

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

I guess
Attachment #787568 - Flags: review?(till) → review+
Comment on attachment 787569 [details] [diff] [review]
blah

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

\o/
Attachment #787569 - Flags: review?(till) → review+
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.
Attachment #787570 - Flags: review?(till) → review+
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!
Attachment #787572 - Flags: review?(till) → review+
(Assignee)

Comment 70

4 years ago
> ::: 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 :)
(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.

Updated

4 years ago
Blocks: 903843
(Assignee)

Comment 72

4 years ago
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
https://hg.mozilla.org/mozilla-central/rev/b9b8ad32c72b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 906626
You need to log in before you can comment on or make changes to this bug.