Last Comment Bug 689362 - mv js/src/{jsvector.h,jshashtable.h} js/public
: mv js/src/{jsvector.h,jshashtable.h} js/public
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on: 684039
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-26 16:26 PDT by Luke Wagner [:luke]
Modified: 2011-10-14 03:55 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
mv files (and rm jsarena.cpp b/c it accidentally was not removed earlier) (71.99 KB, patch)
2011-10-04 14:57 PDT, Luke Wagner [:luke]
wmccloskey: review+
dmandelin: superreview+
Details | Diff | Splinter Review
mv files 2 (184.52 KB, patch)
2011-10-07 16:45 PDT, Luke Wagner [:luke]
wmccloskey: review+
Details | Diff | Splinter Review
kill the limping straggler (15.50 KB, patch)
2011-10-12 10:29 PDT, Luke Wagner [:luke]
wmccloskey: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2011-09-26 16:26:00 PDT
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] 2011-09-26 21:32:19 PDT
Also mfbt/InlineMap.h
Comment 2 Luke Wagner [:luke] 2011-10-04 14:57:08 PDT
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.
Comment 3 Luke Wagner [:luke] 2011-10-07 09:43:31 PDT
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 (:billm) 2011-10-07 11:08:17 PDT
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] 2011-10-07 11:44:43 PDT
(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] 2011-10-07 16:45:29 PDT
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.
Comment 7 Bill McCloskey (:billm) 2011-10-10 11:53:36 PDT
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] 2011-10-10 11:56:41 PDT
(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 11 :Ms2ger 2011-10-12 01:41:33 PDT
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] 2011-10-12 09:22:52 PDT
Hah, yes indeed.
Comment 13 Luke Wagner [:luke] 2011-10-12 10:29:54 PDT
Created attachment 566582 [details] [diff] [review]
kill the limping straggler
Comment 15 Justin Lebar (not reading bugmail) 2011-10-12 20:08:49 PDT
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, back in August) 2011-10-12 20:48:19 PDT
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] (Away 6-20 Aug) 2011-10-13 02:43:07 PDT
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] (Away 6-20 Aug) 2011-10-13 07:01:57 PDT
Since this was not the culprit I relanded it
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a56d92203de
Comment 19 Luke Wagner [:luke] 2011-10-13 09:12:37 PDT
Thanks Marco
Comment 20 Ed Morley [:emorley] 2011-10-14 03:55:56 PDT
https://hg.mozilla.org/mozilla-central/rev/8a56d92203de

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