Closed
Bug 663245
Opened 13 years ago
Closed 13 years ago
Reduce exposure of jsbool.h, jsiter.h, and jsstr.h in INSTALLED_HEADERS
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla7
People
(Reporter: Waldo, Assigned: Waldo)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 1 obsolete file)
15.15 KB,
patch
|
jimb
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
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.
Attachment #538359 -
Attachment is obsolete: true
Attachment #538382 -
Flags: review?(jimb)
Assignee | ||
Updated•13 years ago
|
Summary: Reduce exposure of jsiter.h in INSTALLED_HEADERS → Reduce exposure of jsbool.h, jsiter.h, and jsstr.h in INSTALLED_HEADERS
Comment 2•13 years ago
|
||
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•13 years ago
|
||
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
Assignee | ||
Comment 4•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
(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.
Assignee | ||
Comment 7•13 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/4d99c0c736d4
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla7
Comment 8•13 years ago
|
||
cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/4d99c0c736d4
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Attachment #538382 -
Flags: review?(jimb) → review+
You need to log in
before you can comment on or make changes to this bug.
Description
•