mv js/src/{jsvector.h,jshashtable.h} js/public

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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

6 years ago
Also mfbt/InlineMap.h
(Assignee)

Comment 2

6 years ago
Created attachment 564679 [details] [diff] [review]
mv files (and rm jsarena.cpp b/c it accidentally was not removed earlier)

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

6 years ago
Summary: mv js/src/{jsvector.h,jshashtable.h} js/src/ds → mv js/src/{jsvector.h,jshashtable.h} js/public
Attachment #564679 - Flags: superreview?(dmandelin) → superreview+
(Assignee)

Comment 3

6 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

6 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

6 years ago
Created attachment 565690 [details] [diff] [review]
mv files 2

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

6 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

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c673621e1e
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/b9c673621e1e

Follow-up: https://hg.mozilla.org/mozilla-central/rev/5676f1cc52c8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
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

6 years ago
Hah, yes indeed.
(Assignee)

Comment 13

6 years ago
Created attachment 566582 [details] [diff] [review]
kill the limping straggler
Attachment #566582 - Flags: review?(wmccloskey)
Attachment #566582 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 14

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c0e6b3f7791
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 → ---
Depends on: 692605
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.
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.
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

6 years ago
Thanks Marco
https://hg.mozilla.org/mozilla-central/rev/8a56d92203de
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.