Closed Bug 891215 Opened 6 years ago Closed 6 years ago

Slim down more -inl.h files

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: njn, Assigned: njn)

References

Details

(Whiteboard: [js:t])

Attachments

(20 files)

1.61 KB, patch
terrence
: review+
Details | Diff | Splinter Review
2.49 KB, patch
terrence
: review+
Details | Diff | Splinter Review
7.33 KB, patch
terrence
: review+
Details | Diff | Splinter Review
10.00 KB, patch
terrence
: review+
Details | Diff | Splinter Review
2.73 KB, patch
terrence
: review+
Details | Diff | Splinter Review
7.13 KB, patch
terrence
: review+
Details | Diff | Splinter Review
9.83 KB, patch
terrence
: review+
Details | Diff | Splinter Review
6.00 KB, patch
terrence
: review+
Details | Diff | Splinter Review
7.80 KB, patch
terrence
: review+
Details | Diff | Splinter Review
3.32 KB, patch
terrence
: review+
Details | Diff | Splinter Review
16.12 KB, patch
terrence
: review+
Details | Diff | Splinter Review
17.39 KB, patch
terrence
: review+
Details | Diff | Splinter Review
12.64 KB, patch
terrence
: review+
Details | Diff | Splinter Review
4.24 KB, patch
terrence
: review+
Details | Diff | Splinter Review
18.56 KB, patch
terrence
: review+
Details | Diff | Splinter Review
10.09 KB, patch
terrence
: review+
Details | Diff | Splinter Review
2.97 KB, patch
terrence
: review+
Details | Diff | Splinter Review
36.71 KB, patch
terrence
: review+
Details | Diff | Splinter Review
11.07 KB, patch
terrence
: review+
Details | Diff | Splinter Review
8.45 KB, patch
terrence
: review+
Details | Diff | Splinter Review
IWYU is going to have a field day once I'm done with this.
Attachment #772452 - Flags: review?(terrence)
BytecodeEmitter::tokenStream() is only used in BytecodeEmitter.cpp.
Attachment #772453 - Flags: review?(terrence)
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.
Attachment #772476 - Flags: review?(terrence)
Note that CheckStrictBinding() was dead.
Attachment #772481 - Flags: review?(terrence)
Whoops, missed part 4.
Attachment #772482 - Flags: review?(terrence)
One function left in SharedContext-inl.h.  How frustrating.
Attachment #772484 - Flags: review?(terrence)
Attachment #772495 - Flags: review?(terrence)
Attachment #772496 - Flags: review?(terrence)
Attachment #772502 - Flags: review?(terrence)
Attachment #772505 - Flags: review?(terrence)
And I forgot part 9.
Attachment #772512 - Flags: review?(terrence)
Attachment #772528 - Flags: review?(terrence)
vm/ArgumentsObject.h seems like a better place for ARGS_LENGTH_MAX anyway.
Attachment #772538 - Flags: review?(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.
Attachment #772452 - Flags: review?(terrence) → review+
Comment on attachment 772453 [details] [diff] [review]
(part 2) - Remove BytecodeEmitter-inl.h.

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

r=me
Attachment #772453 - Flags: review?(terrence) → review+
Comment on attachment 772476 [details] [diff] [review]
(part 3) - Slim down ParseMaps-inl.h.

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

r=me
Attachment #772476 - Flags: review?(terrence) → review+
Comment on attachment 772481 [details] [diff] [review]
(part 5) - Remove Parser-inl.h.

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

r=me
Attachment #772481 - Flags: review?(terrence) → review+
Comment on attachment 772482 [details] [diff] [review]
(part 4) - Slim down ParseNode-inl.h.

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

r=me
Attachment #772482 - Flags: review?(terrence) → review+
Comment on attachment 772484 [details] [diff] [review]
(part 6) - Slim down SharedContext-inl.h.

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

r=me
Attachment #772484 - Flags: review?(terrence) → review+
Comment on attachment 772495 [details] [diff] [review]
(part 7) - Remove FindSCCs-inl.h.

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

r=me
Attachment #772495 - Flags: review?(terrence) → review+
Comment on attachment 772496 [details] [diff] [review]
(part 8) - Slim down CompileInfo-inl.h.

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

r=me
Attachment #772496 - Flags: review?(terrence) → review+
Comment on attachment 772502 [details] [diff] [review]
(part 10) - Slim down IonFrames-inl.h.

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

r=me
Attachment #772502 - Flags: review?(terrence) → review+
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.
Attachment #772505 - Flags: review?(terrence) → review+
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.
Attachment #772512 - Flags: review?(terrence) → review+
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.
Attachment #772528 - Flags: review?(terrence) → review+
Comment on attachment 772538 [details] [diff] [review]
(part 13) - Slim down ArgumentsObject-inl.h.

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

r=me
Attachment #772538 - Flags: review?(terrence) → review+
> 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.
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.
Attachment #772839 - Flags: review?(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 on attachment 772838 [details] [diff] [review]
(part 14) - Slim down GlobalObject-inl.h.

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

r=me
Attachment #772838 - Flags: review?(terrence) → review+
Comment on attachment 772839 [details] [diff] [review]
(part 15) - Slim down Interpreter-inl.h.

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

r=me
Attachment #772839 - Flags: review?(terrence) → review+
Comment on attachment 772848 [details] [diff] [review]
(part 16) - Slim down RegExpObject-inl.h.

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

r=me
Attachment #772848 - Flags: review?(terrence) → review+
Whiteboard: [js:t] → [js:t][leave open]
A minor detour on the way to slimming down vm/RegExpStatics-inl.h.
Attachment #772987 - Flags: review?(terrence)
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.
Attachment #773005 - Flags: review?(terrence)
Attachment #773017 - Flags: review?(terrence)
Attachment #773021 - Flags: review?(terrence)
Comment on attachment 773021 [details] [diff] [review]
(part 20) - Slim down String-inl.h.

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

r=me
Attachment #773021 - Flags: review?(terrence) → review+
Comment on attachment 772987 [details] [diff] [review]
(part 17) - Move SizeOfRegExpStaticsData() into the RegExpStaticsObject class.

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

r=me
Attachment #772987 - Flags: review?(terrence) → review+
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
Attachment #773005 - Flags: review?(terrence) → review+
Comment on attachment 773017 [details] [diff] [review]
(part 19) - Slim down Shape-inl.h.

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

r=me
Attachment #773017 - Flags: review?(terrence) → review+
> Wow, the definition was in an inl?

Yeah.  Crazy, huh?  The vanilla header contained just two forward class decls and a function decl.
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?
> >      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.
You need to log in before you can comment on or make changes to this bug.