Closed Bug 909178 Opened 7 years ago Closed 7 years ago

Make jsclass.h not depend on jsapi.h

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: njn, Assigned: njn)

References

Details

(Whiteboard: [js:t])

Attachments

(2 files)

Making jsclass.h not depend on jsapi.h is critical for reducing the amount of Gecko code that depends on jsapi.h.
This is a necessary precursor to the next patch, and a whole lot that will
follow in other bugs.

The need for IdForward.h is explained in a comment.  Annoying, but I don't see
a better way to do it.
Attachment #795241 - Flags: review?(luke)
This patch moves a bunch of class-related stuff from jsapi.h to jsclass.h, and
renames jsclass.h as js/Class.h.

This allows jsapi.h to now include jsclass.h, rather than the other way around.
This is critical for reducing the number of Gecko files that depend on jsapi.h,
because heaps of Gecko code (esp. DOMJSClass.h) needs class-related stuff but
not other jsapi.h stuff.  Also, it's better to have a big header depend on a
small header than the other way around.

It also makes a lot of sense -- it was weird having half of the class-related
stuff in jsapi.h and half in jsclass.h.
Attachment #795243 - Flags: review?(jwalden+bmo)
Comment on attachment 795241 [details] [diff] [review]
(part 1) - Move |jsid| from jsapi.h into js/Id.h.

Review of attachment 795241 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.cpp
@@ -125,5 @@
>  
> -#ifdef JS_USE_JSID_STRUCT_TYPES
> -const jsid JSID_VOID  = { size_t(JSID_TYPE_VOID) };
> -const jsid JSID_EMPTY = { size_t(JSID_TYPE_OBJECT) };
> -#endif

I'd kinda rather just leave it here rather than make a whole new Id.cpp, unless later patches add to Id.cpp?
Attachment #795241 - Flags: review?(luke) → review+
> I'd kinda rather just leave it here rather than make a whole new Id.cpp,
> unless later patches add to Id.cpp?

They don't.  But I like having .cpp files that correspond to js/*.h files, because they provide a place where the .h file is included before any others, and so guarantees that the .h file is self-sufficient.  See bug 905507.
Also, imagine a glorious future in which jsapi.h has been entirely broken up into smaller headers.  Will jsapi.cpp still remain to hold the code that we didn't want to bother creating separate .cpp files for?
Comment on attachment 795243 [details] [diff] [review]
(part 2) - Make jsclass.h not depend on jsapi.h, and rename it js/Class.h.

Review of attachment 795243 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't look too closely at the contents of js/public/Class.h after this, to see if what was in jsclass.h that wasn't in context (or wasn't touched) should really sensibly be there.  Ditto, in reverse somewhat, for jsapi.h.  Probably it's mostly reasonable as-is, didn't seem worth fine-toothed-combing it.

::: js/public/Class.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 jsclass_h

Please add an overview comment

/* JSClass definition and its component types, plus related interfaces. */

between license block and include-guard.  That'll show up on http://mxr.mozilla.org/mozilla-central/source/js/public/ and help people reading our code understand where things are, slightly.

@@ +267,5 @@
> +typedef void
> +(* JSFinalizeOp)(JSFreeOp *fop, JSObject *obj);
> +
> +// Finalizes external strings created by JS_NewExternalString.
> +typedef struct JSStringFinalizer JSStringFinalizer;

Remove the typedef, we're C++ now.
Attachment #795243 - Flags: review?(jwalden+bmo) → review+
For what it's worth, Id.cpp for those library externs seems fine to me.
(In reply to Nicholas Nethercote [:njn] from comment #4)
> But I like having .cpp files that correspond to js/*.h files,
> because they provide a place where the .h file is included before any
> others, and so guarantees that the .h file is self-sufficient.

Oh, I forgot about that; that's a good reason.
https://hg.mozilla.org/mozilla-central/rev/7f8e99aec954
https://hg.mozilla.org/mozilla-central/rev/f8fec7c369d1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.