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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Bug 684039 will add js/src/ds for the refurbished js::LifoAlloc (was JSArenaPool).  Let's put the other data structures in there.

Comment 1

[Luke Wagner [:luke]]

Also mfbt/InlineMap.h

Comment 2

[Luke Wagner [:luke]]

Attachment: 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.

Comment 3

[Luke Wagner [:luke]]

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!

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 5

[Luke Wagner [:luke]]

(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.

Comment 6

[Luke Wagner [:luke]]

Attachment: 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.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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?

Comment 8

[Luke Wagner [:luke]]

(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.)

Comment 9

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c673621e1e

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b9c673621e1e

Follow-up: https://hg.mozilla.org/mozilla-central/rev/5676f1cc52c8

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 565690 [details] [diff] [review]
mv files 2

Seems like jsstaticcheck.h is empty now, any reason it can't be removed?

Comment 12

[Luke Wagner [:luke]]

Hah, yes indeed.

Comment 13

[Luke Wagner [:luke]]

Attachment: kill the limping straggler

Comment 14

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c0e6b3f7791

Comment 15

[Justin Lebar (not reading bugmail)]

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

Comment 16

[Phil Ringnalda (:philor)]

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

[Marco Bonardo [:mak]]

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

[Marco Bonardo [:mak]]

Since this was not the culprit I relanded it
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a56d92203de

Comment 19

[Luke Wagner [:luke]]

Thanks Marco

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8a56d92203de