Closed Bug 1429206 Opened 5 years ago Closed 5 years ago

Move a few more js/src/*.{h,cpp} files into subdirectories

Categories

(Core :: JavaScript Engine, enhancement, P3)

Product:
Core
Component:
JavaScript Engine
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: jorendorff, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 8 obsolete files)

Part 1: Use js/TypeDecls.h instead of redeclaring certain types
28.79 KB, patch
jandem
: review+
Details | Diff | Splinter Review
Part 3: Rename jsobj* -> vm/JSObject*
109.75 KB, patch
jandem
: review+
Details | Diff | Splinter Review
Part 4: Rename jsfun* -> vm/JSFunction*
34.36 KB, patch
jandem
: review+
Details | Diff | Splinter Review
Part 5: Rename jsscript* -> vm/JSScript*
65.71 KB, patch
jandem
: review+
Details | Diff | Splinter Review
Part 6: Rename jscompartment* -> vm/JSCompartment*
72.61 KB, patch
jandem
: review+
Details | Diff | Splinter Review
Part 7: Rename jscntxt* -> vm/JSContext*
117.56 KB, patch
jandem
: review+
Details | Diff | Splinter Review
Part 8: Rename jsatom* -> vm/JSAtom*
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.
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: nobody → jorendorff
Status: NEW → ASSIGNED
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)
Attachment #8941924 - Flags: review?(jdemooij)
Attachment #8941925 - Flags: review?(jdemooij)
Attachment #8941926 - Flags: review?(jdemooij)
Attachment #8941927 - Flags: review?(jdemooij)
Attachment #8941928 - Flags: review?(jdemooij)
Attachment #8941929 - Flags: review?(jdemooij)
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.
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 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 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 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+
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 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+
Attachment #8941927 - Flags: review?(jdemooij) → review+
Attachment #8941928 - Flags: review?(jdemooij) → review+
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+
> 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)
> Do you think it makes sense to do a mass search-and-replace at some point?

Yes.
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.
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)
status-firefox60: --- → fix-optional
Priority: -- → P3
Depends on: 1437876
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)
Attachment #8941921 - Attachment is obsolete: true
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
Attachment #8950634 - Flags: review?(jdemooij)
Attachment #8941929 - Attachment is obsolete: true
Attachment #8941924 - Attachment is obsolete: true
Attachment #8941925 - Attachment is obsolete: true
Attachment #8941926 - Attachment is obsolete: true
Attachment #8941927 - Attachment is obsolete: true
Attachment #8941928 - Attachment is obsolete: true
OK. Part 2 is gone. Part 1 stays, even though it is really preparation for part 2, because it's a good change anyway.
It will feel weird typing `#include "vm/JSObject.h"` at first, but that will wear off quickly.
Attachment #8950626 - Flags: review?(jdemooij) → review+
Attachment #8950627 - Flags: review?(jdemooij) → review+
Attachment #8950628 - Flags: review?(jdemooij) → review+
Attachment #8950629 - Flags: review?(jdemooij) → review+
Attachment #8950632 - Flags: review?(jdemooij) → review+
Attachment #8950633 - Flags: review?(jdemooij) → review+
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+
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 :)
You need to log in before you can comment on or make changes to this bug.