902917 - Minimize #include statements some more

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

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 [inactive]]

Attachment: blah

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: blah2

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 15

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 18

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 19

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 20

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 21

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 22

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 23

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 24

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 25

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 26

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 27

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 28

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 29

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 30

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 31

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 32

[Nicholas Nethercote [inactive]]

Attachment: blah

jsstrinlines.h is only #included by jsstr.cpp now, so this patch just inlines
it.

Comment 33

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 34

[Nicholas Nethercote [inactive]]

Attachment: blah

Comment 35

[Nicholas Nethercote [inactive]]

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]]

Comment on attachment 787517 [details] [diff] [review]
blah

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

r=me

Comment 37

[Till Schneidereit [:till]]

Comment on attachment 787518 [details] [diff] [review]
blah2

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

r=me

Comment 38

[Till Schneidereit [:till]]

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]]

Comment on attachment 787526 [details] [diff] [review]
blah

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

I like it!

Comment 40

[Till Schneidereit [:till]]

Comment on attachment 787529 [details] [diff] [review]
blah

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

yes

Comment 41

[Till Schneidereit [:till]]

Comment on attachment 787528 [details] [diff] [review]
blah

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

nice

Comment 42

[Till Schneidereit [:till]]

Comment on attachment 787530 [details] [diff] [review]
blah

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

makes sense

Comment 43

[Till Schneidereit [:till]]

Comment on attachment 787534 [details] [diff] [review]
blah

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

indeed

Comment 44

[Till Schneidereit [:till]]

Comment on attachment 787532 [details] [diff] [review]
blah

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

yup

Comment 45

[Till Schneidereit [:till]]

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

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

r=me

Comment 46

[Till Schneidereit [:till]]

Comment on attachment 787539 [details] [diff] [review]
blah

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

totally

Comment 47

[Till Schneidereit [:till]]

Comment on attachment 787541 [details] [diff] [review]
blah

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

neat

Comment 48

[Till Schneidereit [:till]]

Comment on attachment 787546 [details] [diff] [review]
blah

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

right

Comment 49

[Till Schneidereit [:till]]

Comment on attachment 787548 [details] [diff] [review]
blah

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

good

Comment 50

[Till Schneidereit [:till]]

Comment on attachment 787549 [details] [diff] [review]
blah

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

short, but nice

Comment 51

[Till Schneidereit [:till]]

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]]

Comment on attachment 787551 [details] [diff] [review]
blah

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

yes

Comment 53

[Till Schneidereit [:till]]

Comment on attachment 787552 [details] [diff] [review]
blah

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

r=me

Comment 54

[Till Schneidereit [:till]]

Comment on attachment 787553 [details] [diff] [review]
blah

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

Lovely

Comment 55

[Till Schneidereit [:till]]

Comment on attachment 787555 [details] [diff] [review]
blah

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

splendid

Comment 56

[Till Schneidereit [:till]]

Comment on attachment 787556 [details] [diff] [review]
blah

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

agreed

Comment 57

[Till Schneidereit [:till]]

Comment on attachment 787557 [details] [diff] [review]
blah

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

yup

Comment 58

[Till Schneidereit [:till]]

Comment on attachment 787558 [details] [diff] [review]
blah

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

likable

Comment 59

[Till Schneidereit [:till]]

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]]

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]]

Comment on attachment 787562 [details] [diff] [review]
blah

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

beautiful

Comment 62

[Till Schneidereit [:till]]

Comment on attachment 787564 [details] [diff] [review]
blah

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

terrific

Comment 63

[Till Schneidereit [:till]]

Comment on attachment 787565 [details] [diff] [review]
blah

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

yes, yes

Comment 64

[Till Schneidereit [:till]]

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]]

Comment on attachment 787567 [details] [diff] [review]
blah

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

very nice

Comment 66

[Till Schneidereit [:till]]

Comment on attachment 787568 [details] [diff] [review]
blah

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

I guess

Comment 67

[Till Schneidereit [:till]]

Comment on attachment 787569 [details] [diff] [review]
blah

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

\o/

Comment 68

[Till Schneidereit [:till]]

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]]

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 [inactive]]

> ::: 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]]

(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 [inactive]]

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]]

https://hg.mozilla.org/mozilla-central/rev/b9b8ad32c72b