663245 - Reduce exposure of jsbool.h, jsiter.h, and jsstr.h in INSTALLED_HEADERS

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

It's possible without much difficulty to eliminate most places outside the JS engine that use jsiter.h.  I think the only place that uses it after this patch is XrayWrapper.cpp, the fixing of which appears to require non-trivial effort.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Larger patch, builds fine

I started looking at other files as well, and this ended up growing past just jsiter.h.  jsbool.h has no reason to be exposed, so it can just go away entirely.  I can cut down on jsstr.h over-inclusion with some forward-declares and a little use of inlines files to avoid needing it.  jsstr.h similarly still has a long way to go on full removal (mostly from indirect inclusion, it seems -- people are generally pretty good about using JS_GetStringCharsAndLength and such), but it's a small step.

Comment 2

[Jim Blandy :jimb]

Comment on attachment 538382 [details] [diff] [review]
Larger patch, builds fine

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

::: js/src/jstracer.cpp
@@ +14777,5 @@
> +    return lir->insStore(nj::LIR_stp, cursor, iter, offsetof(NativeIterator, props_cursor),
> +                         ACCSET_ITER);
> +}
> +
> +} // namespace tjit

These should go in a js/src/tracejit/Writer-inlines.h. It's bizarre to have their implementations in jstracer.cpp, given that we've segregated out Writer.cpp.

Comment 3

[Brendan Eich [:brendan]]

Writer-inlines.h or Writer-inl.h? We are using only the latter so far, want one way (even if shorter-and-slightly-cryptic).

XrayWrapper buddies cc'ed.

/be

Comment 4

[Jeff Walden [:Waldo]]

There was one other Writer inline in jstracer.cpp, pre-existing.  It's there because that's where njn wanted it put in the bug that introduced ArgumentsObject (bug 652746).  I actually like the Writer-inl.h idea (it was in my original patch there), but that probably means we take it up with njn too.  Is it okay if I go with this for now, then we resolve the jstracer.cpp "pollution" in a followup?

...or not actually, he wanted it in Writer.cpp on second look -- guess I misread his comment or something.  Okay to move them to Writer.cpp?

Comment 5

[Jim Blandy :jimb]

(In reply to comment #3)
> Writer-inlines.h or Writer-inl.h? We are using only the latter so far, want
> one way (even if shorter-and-slightly-cryptic).

Sorry --- meant -inl.h.

Comment 6

[Nicholas Nethercote [inactive]]

(In reply to comment #4)
> 
> ...or not actually, he wanted it in Writer.cpp on second look -- guess I
> misread his comment or something.  Okay to move them to Writer.cpp?

Sounds good to me.  It doesn't matter if these functions aren't inlined.

Comment 7

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/4d99c0c736d4

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/4d99c0c736d4