891215 - Slim down more -inl.h files

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

IWYU is going to have a field day once I'm done with this.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Slim down Iterator-inl.h.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove BytecodeEmitter-inl.h.

BytecodeEmitter::tokenStream() is only used in BytecodeEmitter.cpp.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Slim down ParseMaps-inl.h.

The only surprising thing here is that there were three specializations of
ParseMapPool::acquire(), which I was able to replace with a single generic
definition.  Weird.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Remove Parser-inl.h.

Note that CheckStrictBinding() was dead.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Slim down ParseNode-inl.h.

Whoops, missed part 4.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Slim down SharedContext-inl.h.

One function left in SharedContext-inl.h.  How frustrating.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Remove FindSCCs-inl.h.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Slim down CompileInfo-inl.h.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 10) - Slim down IonFrames-inl.h.

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: (part 11) - Remove PcScriptCache-inl.h.

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: (part 9) - Slim down IonFrameIterator-inl.h.

And I forgot part 9.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: (part 12) - Slim down ScopeObject-inl.h.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (part 13) - Slim down ArgumentsObject-inl.h.

vm/ArgumentsObject.h seems like a better place for ARGS_LENGTH_MAX anyway.

Comment 14

[Terrence Cole [:terrence]]

Comment on attachment 772452 [details] [diff] [review]
(part 1) - Slim down Iterator-inl.h.

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

r=me

::: js/src/jsiter.h
@@ +119,5 @@
>    public:
>      static Class class_;
>  
> +    NativeIterator *getNativeIterator() const {
> +        return static_cast<js::NativeIterator *>(getPrivate());

It's a shame this needs to drop the assertion. Probably not worth adding an out-of-line path for it since anything that uses the return is going to go off the rails quickly.

Comment 15

[Terrence Cole [:terrence]]

Comment on attachment 772453 [details] [diff] [review]
(part 2) - Remove BytecodeEmitter-inl.h.

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

r=me

Comment 16

[Terrence Cole [:terrence]]

Comment on attachment 772476 [details] [diff] [review]
(part 3) - Slim down ParseMaps-inl.h.

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

r=me

Comment 17

[Terrence Cole [:terrence]]

Comment on attachment 772481 [details] [diff] [review]
(part 5) - Remove Parser-inl.h.

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

r=me

Comment 18

[Terrence Cole [:terrence]]

Comment on attachment 772482 [details] [diff] [review]
(part 4) - Slim down ParseNode-inl.h.

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

r=me

Comment 19

[Terrence Cole [:terrence]]

Comment on attachment 772484 [details] [diff] [review]
(part 6) - Slim down SharedContext-inl.h.

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

r=me

Comment 20

[Terrence Cole [:terrence]]

Comment on attachment 772495 [details] [diff] [review]
(part 7) - Remove FindSCCs-inl.h.

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

r=me

Comment 21

[Terrence Cole [:terrence]]

Comment on attachment 772496 [details] [diff] [review]
(part 8) - Slim down CompileInfo-inl.h.

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

r=me

Comment 22

[Terrence Cole [:terrence]]

Comment on attachment 772502 [details] [diff] [review]
(part 10) - Slim down IonFrames-inl.h.

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

r=me

Comment 23

[Terrence Cole [:terrence]]

Comment on attachment 772505 [details] [diff] [review]
(part 11) - Remove PcScriptCache-inl.h.

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

r=me

::: js/src/ion/PcScriptCache.h
@@ +44,2 @@
>      bool get(JSRuntime *rt, uint32_t hash, uint8_t *addr,
> +             JSScript **scriptRes, jsbytecode **pcRes) {

Brace on new line, since you're here.

Comment 24

[Terrence Cole [:terrence]]

Comment on attachment 772512 [details] [diff] [review]
(part 9) - Slim down IonFrameIterator-inl.h.

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

r=me

::: js/src/ion/IonFrameIterator.h
@@ +214,5 @@
> +
> +    Value *argv = actualArgs();
> +    for (unsigned i = start; i < end; i++)
> +        op(argv[i]);
> +}

The indentation of forEachCanonicalActualArg looks wrong.

@@ +262,5 @@
>      }
>  
>      template <class Op>
> +    void readFrameArgs(Op &op, const Value *argv, Value *scopeChain, Value *thisv,
> +                       unsigned start, unsigned formalEnd, unsigned iterEnd, JSScript *script) {

Brace on new line.

Comment 25

[Terrence Cole [:terrence]]

Comment on attachment 772528 [details] [diff] [review]
(part 12) - Slim down ScopeObject-inl.h.

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

r=me

::: js/src/vm/ScopeObject.h
@@ +336,5 @@
>    protected:
>      /* Blocks contain an object slot for each slot i: 0 <= i < slotCount. */
> +    const Value &slotValue(unsigned i) {
> +        return getSlotRef(RESERVED_SLOTS + i);
> +

Remove extra newline.

Comment 26

[Terrence Cole [:terrence]]

Comment on attachment 772538 [details] [diff] [review]
(part 13) - Slim down ArgumentsObject-inl.h.

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

r=me

Comment 27

[Nicholas Nethercote [inactive]]

> r=me

FWIW, if you don't write any comments, then the r+ shows up in Bugzilla as one of those small pseudo-comments, rather than a full comment.  Maybe you do this precisely so you get a full comment.


> > +    NativeIterator *getNativeIterator() const {
> > +        return static_cast<js::NativeIterator *>(getPrivate());
> 
> It's a shame this needs to drop the assertion.

That's a feature, not a bug!  getNativeIterator() is a method of PropertyIteratorObject, which means the assertion is vacuous -- unless someone is doing something horrible with a cast, they must have done as<PropertyIteratorObject>() first, which itself has that assertion.  (I checked the code, and all calls to getNativeIterator() meet this property.)

I've removed quite a few is<FooObject>() assertions within FooObject methods as part of my recent is<FooObject>/as<FooObject> clean-ups.

Comment 28

[Nicholas Nethercote [inactive]]

BTW, apologies for asking you to do so many reviews, but I see they're only taking you a short time each.  There are a few more to come.

Comment 29

[Nicholas Nethercote [inactive]]

Attachment: (part 14) - Slim down GlobalObject-inl.h.

Comment 30

[Nicholas Nethercote [inactive]]

Attachment: (part 15) - Slim down Interpreter-inl.h.

Comment 31

[Nicholas Nethercote [inactive]]

Attachment: (part 16) - Slim down RegExpObject-inl.h.

Comment 32

[Terrence Cole [:terrence]]

(In reply to Nicholas Nethercote [:njn] from comment #27)
> > r=me
> 
> FWIW, if you don't write any comments, then the r+ shows up in Bugzilla as
> one of those small pseudo-comments, rather than a full comment.  Maybe you
> do this precisely so you get a full comment.

Yes, I've been annoyed in the past when I missed an r+ that was mixed in with other pseudo-comments. It also reduces ambiguity when a followup post that carries the r+ drops the originator of the r+. I can't claim credit for the idea though: I saw Boris doing it and immediately adopted it for the above reasons.

(In reply to Nicholas Nethercote [:njn] from comment #28)
> BTW, apologies for asking you to do so many reviews, but I see they're only
> taking you a short time each.  There are a few more to come.

Yes, when the diffs are mostly copies, it more than halves the review time.

Note: Yes, I am assuming you aren't maliciously injecting code here.

Comment 33

[Terrence Cole [:terrence]]

Comment on attachment 772838 [details] [diff] [review]
(part 14) - Slim down GlobalObject-inl.h.

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

r=me

Comment 34

[Terrence Cole [:terrence]]

Comment on attachment 772839 [details] [diff] [review]
(part 15) - Slim down Interpreter-inl.h.

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

r=me

Comment 35

[Terrence Cole [:terrence]]

Comment on attachment 772848 [details] [diff] [review]
(part 16) - Slim down RegExpObject-inl.h.

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

r=me

Comment 36

[Nicholas Nethercote [inactive]]

Parts 1--13:
https://hg.mozilla.org/integration/mozilla-inbound/rev/144173ec5cf8
https://hg.mozilla.org/integration/mozilla-inbound/rev/81cf3ae0154b
https://hg.mozilla.org/integration/mozilla-inbound/rev/260f3e4be30e
https://hg.mozilla.org/integration/mozilla-inbound/rev/20596578a059
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5da543cc0ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/517f46403ff6
https://hg.mozilla.org/integration/mozilla-inbound/rev/defc6801c431
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fea433d4a88
https://hg.mozilla.org/integration/mozilla-inbound/rev/f15355fab6b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/1eeb9038a57d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2882076c2af
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed09fcb80ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/79d75a5b45cc

Comment 37

[Nicholas Nethercote [inactive]]

Attachment: (part 17) - Move SizeOfRegExpStaticsData() into the RegExpStaticsObject class.

A minor detour on the way to slimming down vm/RegExpStatics-inl.h.

Comment 38

[Nicholas Nethercote [inactive]]

Attachment: (part 18) - Slim down RegExpStatics-inl.h.

So this is a weird one.  vm/RegExpStatics.h contains nothing but a couple of
forward declarations, and vm/RegExpStatics-inl.h contains class RegExpStatics
and all the related junk.  I was able to everything but three methods from
vm/RegExpStatics-inl.h into vm/RegExpStatics.h.

Comment 39

[Nicholas Nethercote [inactive]]

Attachment: (part 19) - Slim down Shape-inl.h.

Comment 40

[Nicholas Nethercote [inactive]]

Attachment: (part 20) - Slim down String-inl.h.

Comment 41

[Terrence Cole [:terrence]]

Comment on attachment 773021 [details] [diff] [review]
(part 20) - Slim down String-inl.h.

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

r=me

Comment 42

[Terrence Cole [:terrence]]

Comment on attachment 772987 [details] [diff] [review]
(part 17) - Move SizeOfRegExpStaticsData() into the RegExpStaticsObject class.

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

r=me

Comment 43

[Terrence Cole [:terrence]]

Comment on attachment 773005 [details] [diff] [review]
(part 18) - Slim down RegExpStatics-inl.h.

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

Wow, the definition was in an inl?

r=me

Comment 44

[Terrence Cole [:terrence]]

Comment on attachment 773017 [details] [diff] [review]
(part 19) - Slim down Shape-inl.h.

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

r=me

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/144173ec5cf8
https://hg.mozilla.org/mozilla-central/rev/81cf3ae0154b
https://hg.mozilla.org/mozilla-central/rev/260f3e4be30e
https://hg.mozilla.org/mozilla-central/rev/20596578a059
https://hg.mozilla.org/mozilla-central/rev/a5da543cc0ef
https://hg.mozilla.org/mozilla-central/rev/517f46403ff6
https://hg.mozilla.org/mozilla-central/rev/defc6801c431
https://hg.mozilla.org/mozilla-central/rev/2fea433d4a88
https://hg.mozilla.org/mozilla-central/rev/f15355fab6b4
https://hg.mozilla.org/mozilla-central/rev/1eeb9038a57d
https://hg.mozilla.org/mozilla-central/rev/e2882076c2af
https://hg.mozilla.org/mozilla-central/rev/8ed09fcb80ce
https://hg.mozilla.org/mozilla-central/rev/79d75a5b45cc

Comment 46

[Nicholas Nethercote [inactive]]

> Wow, the definition was in an inl?

Yeah.  Crazy, huh?  The vanilla header contained just two forward class decls and a function decl.

Comment 47

[Nicholas Nethercote [inactive]]

Parts 14--20:
https://hg.mozilla.org/integration/mozilla-inbound/rev/122be936149b
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8f8b03cdb36
https://hg.mozilla.org/integration/mozilla-inbound/rev/763c1a2daaee
https://hg.mozilla.org/integration/mozilla-inbound/rev/42d3202f0e03
https://hg.mozilla.org/integration/mozilla-inbound/rev/c56ec9d10222
https://hg.mozilla.org/integration/mozilla-inbound/rev/01d11112cb0c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e166f90a128f

Comment 48

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/122be936149b
https://hg.mozilla.org/mozilla-central/rev/a8f8b03cdb36
https://hg.mozilla.org/mozilla-central/rev/763c1a2daaee
https://hg.mozilla.org/mozilla-central/rev/42d3202f0e03
https://hg.mozilla.org/mozilla-central/rev/c56ec9d10222
https://hg.mozilla.org/mozilla-central/rev/01d11112cb0c
https://hg.mozilla.org/mozilla-central/rev/e166f90a128f

Comment 49

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 772452 [details] [diff] [review]
(part 1) - Slim down Iterator-inl.h.

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

::: js/src/builtin/Iterator-inl.h
@@ +13,4 @@
>  inline void
>  js::PropertyIteratorObject::setNativeIterator(js::NativeIterator *ni)
>  {
>      setPrivate(ni);

Can't this be in the main header?

Comment 50

[Nicholas Nethercote [inactive]]

> >      setPrivate(ni);
> 
> Can't this be in the main header?

Nope.  setPrivate() is in vm/ObjectImpl-inl.h because -- like so many other functions in -inl.h files -- it involves object write barriers.