Closed
Bug 1429206
Opened 8 years ago
Closed 7 years ago
Move a few more js/src/*.{h,cpp} files into subdirectories
Categories
(Core :: JavaScript Engine, enhancement, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: jorendorff, Assigned: jorendorff)
References
Details
Attachments
(7 files, 8 obsolete files)
28.79 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
109.75 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
34.36 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
65.71 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
72.61 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
117.56 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
45.53 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
I wrote patches for a few of these over the vacation. They don't bitrot patches as much as I expected, so we can just land them.
Assignee | ||
Comment 1•8 years ago
|
||
This adds JS::Zone to TypeDecls.h. Arguably that is pretty borderline, but
even though the Zone type is only used in a dozen or so places in Gecko,
it's a central concept.
Attachment #8941921 -
Flags: review?(jdemooij)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
The old type names still exist (as aliases for the new ones), and we still
mainly use the old names rather than the new ones.
The affected types are JSContext, JSCompartment, JSFunction, JSObject, JSScript,
and JSString.
We have some places where functions with the same name are declared
in multiple namespaces. In a few of those cases, moving JSObject into the
JS namespace interacts with C++ argument-dependent lookup to cause issues,
which I fixed by adding `::` before some function names.
Attachment #8941923 -
Flags: review?(jdemooij)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8941924 -
Flags: review?(jdemooij)
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8941925 -
Flags: review?(jdemooij)
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8941926 -
Flags: review?(jdemooij)
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8941927 -
Flags: review?(jdemooij)
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8941928 -
Flags: review?(jdemooij)
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #8941929 -
Flags: review?(jdemooij)
Assignee | ||
Comment 9•8 years ago
|
||
That's it for this batch. In a follow-up, I also want to move:
* vm/DateObject* -> builtin/
* vm/Shape* -> obj/, along with ArrayObject, ArrayBufferObject,
NativeObject, TypedArrayObject, and probably some others
The idea is that the core classes and methods about how we
represent objects in memory go in js/src/obj.
The remaining headers in js/src are a mix, but it'd be more work to sort them:
* jsalloc.h, jsapi.h, jsfriendapi.h, jstypes.h, and 7 others are public headers.
They should all be moved to js/public, with or without renaming them.
* jsarray* should be taken apart, the contents split across several headers.
It's more work than just renaming the files.
* jsgc* -> gc/
* json.* -> builtin/
* jsbool*.h, jsdate.h, jsexn.h, jsiter.h, jsnum.h, jsstr.h
All of these could plausibly go in builtin/ or vm/.
* jshashutil.h -> ds/ probably
* jsopcode*.h belongs in vm/ ...but there's already a vm/Opcodes.h
* jsdtoa.h, jsnspr.h, jsutil.h, jswin.h, NamespaceImports.h are low-level
and/or defy categorization; they could be in their own directory maybe.
Comment 10•8 years ago
|
||
To help understanding, could you provide the reasoning for having builtin/, vm/ and new directories you're introducing here, as well as criteria that would help us deciding where to put files, please? I've always found it confusing to decide (builtin/ vs vm/) and I would love it if there was a detailed explanation somewhere of what must go where. Thanks!
Comment 11•8 years ago
|
||
Comment on attachment 8941921 [details] [diff] [review]
Part 1: Use js/TypeDecls.h instead of redeclaring certain types
Review of attachment 8941921 [details] [diff] [review]:
-----------------------------------------------------------------
Yes, much nicer.
Attachment #8941921 -
Flags: review?(jdemooij) → review+
Comment 12•8 years ago
|
||
Comment on attachment 8941923 [details] [diff] [review]
Part 2: Rename JSObject -> JS::Object (and several others)
Review of attachment 8941923 [details] [diff] [review]:
-----------------------------------------------------------------
Do you think it makes sense to do a mass search-and-replace at some point?
Attachment #8941923 -
Flags: review?(jdemooij) → review+
Comment 13•8 years ago
|
||
Comment on attachment 8941924 [details] [diff] [review]
Part 3: Rename jsobj* -> obj/Object*
Review of attachment 8941924 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/gc/Marking.h
@@ +178,5 @@
> inline void
> MakeAccessibleAfterMovingGC(void* anyp) {}
>
> inline void
> +MakeAccessibleAfterMovingGC(JSObject* obj); // Defined in objects/JSObject-inl.h.
Defined in obj/Object-inl.h.
::: js/src/json.cpp
@@ +13,5 @@
> #include "jsarray.h"
> #include "jsatom.h"
> #include "jscntxt.h"
> #include "jsnum.h"
> +
Remove the blank line here
Attachment #8941924 -
Flags: review?(jdemooij) → review+
Comment 14•8 years ago
|
||
Hm I don't understand part 4 - doesn't JSFunction (JS::Function) belong in obj/ instead of vm/? Why would TypedArrayObject and ArrayObject be in obj/ but not Function?
It would be nice to have a simple rule here; maybe everything object related + everything with a Class* should be in obj/?
Flags: needinfo?(jorendorff)
Comment 15•8 years ago
|
||
Comment on attachment 8941926 [details] [diff] [review]
Part 5: Rename jsscript* -> vm/Script*
Review of attachment 8941926 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/vm/Script-inl.h
@@ +3,5 @@
> * This Source Code Form is subject to the terms of the Mozilla Public
> * License, v. 2.0. If a copy of the MPL was not distributed with this
> * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>
> +#ifndef vm_JSScript_inl_h
vm_Script_inl_h and below
::: js/src/vm/Script.h
@@ +5,5 @@
> * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>
> /* JS script descriptor. */
>
> +#ifndef vm_JSScript_h
And here.
Attachment #8941926 -
Flags: review?(jdemooij) → review+
Updated•8 years ago
|
Attachment #8941927 -
Flags: review?(jdemooij) → review+
Updated•8 years ago
|
Attachment #8941928 -
Flags: review?(jdemooij) → review+
Comment 16•8 years ago
|
||
Comment on attachment 8941929 [details] [diff] [review]
Part 8: Rename jsatom* -> vm/Atom*
Review of attachment 8941929 [details] [diff] [review]:
-----------------------------------------------------------------
Nice.
::: js/src/vm/Atom-inl.h
@@ +3,5 @@
> * This Source Code Form is subject to the terms of the Mozilla Public
> * License, v. 2.0. If a copy of the MPL was not distributed with this
> * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>
> #ifndef jsatominlines_h
Nit: vm_Atom_inl_h and vm_Atom_h in vm/Atom.h
Attachment #8941929 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 17•8 years ago
|
||
> Hm I don't understand part 4 - doesn't JSFunction (JS::Function) belong in obj/ instead of vm/? Why would TypedArrayObject and ArrayObject be in obj/ but not Function?
>
> It would be nice to have a simple rule here; maybe everything object related + everything with a Class* should be in obj/?
OK, discussed this with jandem and evilpies. Forget obj/, I'll move jsobj.h -> vm/Object.h.
I wrote up our other conclusions in the wiki: https://wiki.mozilla.org/JavaScript:SpiderMonkey:Directories
Flags: needinfo?(jorendorff)
Assignee | ||
Comment 18•8 years ago
|
||
> Do you think it makes sense to do a mass search-and-replace at some point?
Yes.
Assignee | ||
Comment 19•8 years ago
|
||
Having said that...
Interestingly, renaming JSObject to JS::Object in patch 2 causes problems. SM compiles but fails this jsapi-test:
testIncrementalWeakCacheSweeping
Assertion failure: !js::gc::EdgeNeedsSweepUnbarrieredSlow(&obj), at /Users/jorendorff/work/gecko/js/src/build_DBG.OBJ/dist/include/js/HeapAPI.h:595
I imagine this is due to C++ argument-dependent lookup, but I don't know an easy way to track down something like that. I am going to push everything else to try server, and set patch 2 aside for now.
Assignee | ||
Comment 20•8 years ago
|
||
Comment 21•7 years ago
|
||
Comment on attachment 8941925 [details] [diff] [review]
Part 4: Rename jsfun* -> vm/Function*
Cancelling review since I assume there will be an updated patch.
Attachment #8941925 -
Flags: review?(jdemooij)
Assignee | ||
Updated•7 years ago
|
status-firefox60:
--- → fix-optional
Priority: -- → P3
Assignee | ||
Comment 22•7 years ago
|
||
Assignee | ||
Comment 23•7 years ago
|
||
Assignee | ||
Comment 24•7 years ago
|
||
Assignee | ||
Comment 25•7 years ago
|
||
Assignee | ||
Comment 26•7 years ago
|
||
This adds JS::Zone to TypeDecls.h. Arguably that is pretty borderline, but
even though the Zone type is only used in a dozen or so places in Gecko,
it's a central concept.
Attachment #8950626 -
Flags: review?(jdemooij)
Assignee | ||
Updated•7 years ago
|
Attachment #8941921 -
Attachment is obsolete: true
Assignee | ||
Comment 27•7 years ago
|
||
Attachment #8950627 -
Flags: review?(jdemooij)
Assignee | ||
Comment 28•7 years ago
|
||
Comment on attachment 8941923 [details] [diff] [review]
Part 2: Rename JSObject -> JS::Object (and several others)
Obsoleting this patch: This change breaks everything, and tracking down why is too hard right now. Maybe someday.
Attachment #8941923 -
Attachment is obsolete: true
Assignee | ||
Comment 29•7 years ago
|
||
Attachment #8950628 -
Flags: review?(jdemooij)
Assignee | ||
Comment 30•7 years ago
|
||
Attachment #8950629 -
Flags: review?(jdemooij)
Assignee | ||
Comment 31•7 years ago
|
||
Attachment #8950632 -
Flags: review?(jdemooij)
Assignee | ||
Comment 32•7 years ago
|
||
Attachment #8950633 -
Flags: review?(jdemooij)
Assignee | ||
Comment 33•7 years ago
|
||
Attachment #8950634 -
Flags: review?(jdemooij)
Assignee | ||
Updated•7 years ago
|
Attachment #8941929 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8941924 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8941925 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8941926 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8941927 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8941928 -
Attachment is obsolete: true
Assignee | ||
Comment 34•7 years ago
|
||
OK. Part 2 is gone. Part 1 stays, even though it is really preparation for part 2, because it's a good change anyway.
Assignee | ||
Comment 35•7 years ago
|
||
It will feel weird typing `#include "vm/JSObject.h"` at first, but that will wear off quickly.
Updated•7 years ago
|
Attachment #8950626 -
Flags: review?(jdemooij) → review+
Updated•7 years ago
|
Attachment #8950627 -
Flags: review?(jdemooij) → review+
Updated•7 years ago
|
Attachment #8950628 -
Flags: review?(jdemooij) → review+
Updated•7 years ago
|
Attachment #8950629 -
Flags: review?(jdemooij) → review+
Updated•7 years ago
|
Attachment #8950632 -
Flags: review?(jdemooij) → review+
Updated•7 years ago
|
Attachment #8950633 -
Flags: review?(jdemooij) → review+
Comment 36•7 years ago
|
||
Comment on attachment 8950634 [details] [diff] [review]
Part 8: Rename jsatom* -> vm/JSAtom*
Review of attachment 8950634 [details] [diff] [review]:
-----------------------------------------------------------------
Nice!
Attachment #8950634 -
Flags: review?(jdemooij) → review+
Comment 37•7 years ago
|
||
Note that JSString is currently in vm/String.h, so we could have vm/Function.h instead of vm/JSFunction.h I don't care too much though; either one is better than jsfun.h :)
Assignee | ||
Comment 38•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/301c61b644c24acc65c4d0b379736d1d3c7999a6
Bug 1429206 - Part 1: Use js/TypeDecls.h instead of redeclaring certain types. r=jandem.
https://hg.mozilla.org/integration/mozilla-inbound/rev/eabb74b1c3bd181ff74c9d41003bc2b9aee6fe68
Bug 1429206 - Part 3: Rename jsobj* -> vm/JSObject*. r=jandem.
https://hg.mozilla.org/integration/mozilla-inbound/rev/23d76102a2e8acd0e79c26faa44eec833db58529
Bug 1429206 - Part 4: Rename jsfun* -> vm/JSFunction*.
https://hg.mozilla.org/integration/mozilla-inbound/rev/711c111e333087942d9e5c7310a342ddffbcfabb
Bug 1429206 - Part 5: Rename jsscript* -> vm/JSScript*. r=jandem.
https://hg.mozilla.org/integration/mozilla-inbound/rev/212a88df0f3d598edf04df824d568667c20c5fec
Bug 1429206 - Part 6: Rename jscompartment* -> vm/JSCompartment*. r=jandem.
https://hg.mozilla.org/integration/mozilla-inbound/rev/609eb4880073d3b1ff1a198ec68737dce50bcb3f
Bug 1429206 - Part 7: Rename jscntxt* -> vm/JSContext*. r=jandem.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0054d892b106cfc60874dc1662a7278c74108b03
Bug 1429206 - Part 8: Rename jsatom* -> vm/JSAtom*. r=jandem.
Comment 39•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/301c61b644c2
https://hg.mozilla.org/mozilla-central/rev/eabb74b1c3bd
https://hg.mozilla.org/mozilla-central/rev/23d76102a2e8
https://hg.mozilla.org/mozilla-central/rev/711c111e3330
https://hg.mozilla.org/mozilla-central/rev/212a88df0f3d
https://hg.mozilla.org/mozilla-central/rev/609eb4880073
https://hg.mozilla.org/mozilla-central/rev/0054d892b106
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•