Last Comment Bug 663245 - Reduce exposure of jsbool.h, jsiter.h, and jsstr.h in INSTALLED_HEADERS
: Reduce exposure of jsbool.h, jsiter.h, and jsstr.h in INSTALLED_HEADERS
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 554088
  Show dependency treegraph
 
Reported: 2011-06-09 14:53 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-01-07 07:22 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (5.43 KB, patch)
2011-06-09 14:53 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Larger patch, builds fine (15.15 KB, patch)
2011-06-09 16:35 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
jimb: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-09 14:53:55 PDT
Created attachment 538359 [details] [diff] [review]
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] (remove +bmo to email) 2011-06-09 16:35:59 PDT
Created attachment 538382 [details] [diff] [review]
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 2011-06-14 11:57:59 PDT
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] 2011-06-14 12:04:52 PDT
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] (remove +bmo to email) 2011-06-14 12:10:33 PDT
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 2011-06-14 12:13:38 PDT
(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 [:njn] 2011-06-14 20:11:32 PDT
(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] (remove +bmo to email) 2011-06-16 13:15:10 PDT
http://hg.mozilla.org/tracemonkey/rev/4d99c0c736d4
Comment 8 Chris Leary [:cdleary] (not checking bugmail) 2011-06-20 17:09:37 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/4d99c0c736d4

Note You need to log in before you can comment on or make changes to this bug.