Closed
Bug 689362
Opened 13 years ago
Closed 13 years ago
mv js/src/{jsvector.h,jshashtable.h} js/public
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla10
People
(Reporter: luke, Assigned: luke)
References
Details
Attachments
(2 files, 1 obsolete file)
184.52 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
15.50 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
Bug 684039 will add js/src/ds for the refurbished js::LifoAlloc (was JSArenaPool). Let's put the other data structures in there.
Assignee | ||
Comment 1•13 years ago
|
||
Also mfbt/InlineMap.h
Assignee | ||
Comment 2•13 years ago
|
||
After some discussion, here's the proposal: Add a new dir js/public which contains headers which we export to mozilla and have no intention of removing as part of INSTALLED_HEADER-evisceration. Initial contents (in this patch) are: - Vector.h (was jsvector.h) - HashTable.h (was jshashtable.h) - Tl.h (was jstl.h) When we have finished eviscerating INSTALLED_HEADERS, it stands to reason that the remaining exported files (jsapi.h, jsutil.h, jstypes.h, ...) can be placed inside js/public giving us a clear separation of exported vs. not.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #564679 -
Flags: superreview?(dmandelin)
Attachment #564679 -
Flags: review?
Assignee | ||
Updated•13 years ago
|
Summary: mv js/src/{jsvector.h,jshashtable.h} js/src/ds → mv js/src/{jsvector.h,jshashtable.h} js/public
Updated•13 years ago
|
Attachment #564679 -
Flags: superreview?(dmandelin) → superreview+
Assignee | ||
Comment 3•13 years ago
|
||
Comment on attachment 564679 [details] [diff] [review] mv files (and rm jsarena.cpp b/c it accidentally was not removed earlier) Silent r? name lookup failure strikes again!
Attachment #564679 -
Flags: review? → review?(wmccloskey)
Comment on attachment 564679 [details] [diff] [review] mv files (and rm jsarena.cpp b/c it accidentally was not removed earlier) Review of attachment 564679 [details] [diff] [review]: ----------------------------------------------------------------- Nice patch. I like how this lays out a long-term path for separating API from non-API stuff. I think jsweakmap.h relies on hashtables, so it makes sense to add an #include for it there (I assume it's picking it up from some other include). It seems a bit weird to me that js/public/Vector.h include jsbit.h. I guess we'll want to move jsbit.h to public eventually? I don't like the name js/Tl.h too much. jstl.h was clever, but js/Tl.h seems like a stretch. What about js/Templates.h or js/TemplateLib.h? ::: js/src/Makefile.in @@ +230,5 @@ > VPATH += \ > $(srcdir)/vm \ > $(srcdir)/frontend \ > $(srcdir)/ds \ > $(NULL) It seems like these will now be added to VPATH twice.
Attachment #564679 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 5•13 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #4) > It seems a bit weird to me that js/public/Vector.h include jsbit.h. I guess > we'll want to move jsbit.h to public eventually? That's right; we need to jsbit.h and jsutil.h into a portion we actually want to export and a never-exported portion. Probably we can jsbit into jsutil and then split the result into js/public/Util.h and a (private) js/src/jsutil.h. > I don't like the name js/Tl.h too much. jstl.h was clever, but js/Tl.h seems > like a stretch. What about js/Templates.h or js/TemplateLib.h? Yeah, I was on the fence myself. js/TemplateLib.h sounds nice; I'll do that.
Assignee | ||
Comment 6•13 years ago
|
||
I actually did deal with jsutil.h and jsbit.h in the way I described. I would have had js/public/Util.h and js/src/jsutil.h but whatever "namespacing" scheme the build system uses has a conflict with mozilla/Util.h. Thus, I named it js/public/Utility.h. Now, engine-wide internal utilities can go in jsutil.h and (eventually) will stop getting spread outside the JS engine (once will finish off INSTALLED_HEADERS). The only header left that is included by js/public but not in js/public is jstypes.h (and the files it includes). Eventually, I think this will go into mozilla/Types.h (which currently just #include's jstypes.h) so for now I'll just #include mozilla/Types.h from inside js/public.
Attachment #564679 -
Attachment is obsolete: true
Attachment #565690 -
Flags: review?(wmccloskey)
Comment on attachment 565690 [details] [diff] [review] mv files 2 I just gave this a quick look. I assume we'll try to pare down Utility.h over time? Also, can't jsutil.h be removed from INSTALLED_HEADERS?
Attachment #565690 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 8•13 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #7) > I just gave this a quick look. I assume we'll try to pare down Utility.h > over time? Hopefully. I did a bit of pairing already. > Also, can't jsutil.h be removed from INSTALLED_HEADERS? Not until the only thing we export is js/public. (That is, jsutil.h is still included by INSTALLED_HEADERS which we intend to uninstall.)
Assignee | ||
Comment 9•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c673621e1e
Target Milestone: --- → mozilla10
Comment 10•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b9c673621e1e Follow-up: https://hg.mozilla.org/mozilla-central/rev/5676f1cc52c8
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 11•13 years ago
|
||
Comment on attachment 565690 [details] [diff] [review] mv files 2 Seems like jsstaticcheck.h is empty now, any reason it can't be removed?
Assignee | ||
Comment 12•13 years ago
|
||
Hah, yes indeed.
Assignee | ||
Comment 13•13 years ago
|
||
Attachment #566582 -
Flags: review?(wmccloskey)
Attachment #566582 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 14•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c0e6b3f7791
Comment 15•13 years ago
|
||
Backed out due to suspected macos x64 opt moth permaorange. Which is very strange, considering the contents of this patch. https://hg.mozilla.org/integration/mozilla-inbound/rev/26fff9d82083
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 16•13 years ago
|
||
Or perhaps not so strange, given the existence of bug 692605 and js/src/'s history with things accessing uninitialized memory that fail after changes like this which couldn't possibly cause them to fail.
Comment 17•13 years ago
|
||
I retriggered a couple oth on the push before this one and they are now orange, so it's unlikely this may have been the cause.
Comment 18•13 years ago
|
||
Since this was not the culprit I relanded it https://hg.mozilla.org/integration/mozilla-inbound/rev/8a56d92203de
No longer depends on: 692605
Assignee | ||
Comment 19•13 years ago
|
||
Thanks Marco
Comment 20•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8a56d92203de
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•