Closed Bug 911020 Opened 6 years ago Closed 6 years ago

Introduce js/TypeDecls.h, which holds very commonly used JS engine type declarations.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(1 file)

Forward declarations of classes like |JSContext| and |JS::Value| are great in
Gecko.  But we've decided to use typedefs like |JS::HandleObject| everywhere,
so Gecko will need to see those typedefs.

Since lots of Gecko code only needs type declarations for a small number of
SpiderMonkey types (e.g. JSObject, Value, JSContext, HandleObject, HandleValue,
jschar), I think it's worth having a header for such type declarations.

"What about jspubtd.h?" I hear you ask.  Well, jspubtd.h provides much the same
service, but it's grown far too big, IMO.  It has lots of types that don't need
this level of exposure, and even defines several classes and structs.  Ugh.  So
I want a new, slimmer header of this type, and this can be viewed as a first
step towards eliminating jspubtd.h.
This patch introduces js/TypeDecls.h, which contains a bespoke, hand-selected
collection of only 22 of the finest, most widely-applicable type declarations
that SpiderMonkey has to offer.

The patch also replaces most forward declarations of any of those 22 types with
a #include of TypeDecls.h.  There were also a few files where I could remove
the forward decls without including TypeDecls.h, e.g. because the type was no
longer used.

Since |jsid| is one of the 22 types, I was able to remove js/IdForward.h, which
is nice.

I also got rid of |HandleModule|, because it's not used, and moved
|RootedModule|'s definition to builtin/Module.cpp.  And I moved
|{Handle,Rooted}ScriptSource| to gc/Rooting.h, because they don't need to be
visible outside SM.

The End.
Attachment #797662 - Flags: review?(luke)
(In reply to Nicholas Nethercote [:njn] from comment #1)
> This patch introduces js/TypeDecls.h, which contains a bespoke, hand-selected
> collection of only 22 of the finest, most widely-applicable type declarations
> that SpiderMonkey has to offer.

lol.  Seriously, though, we'll need to hold the line to keep TypeDecls.h slim.  And now our watch begins.
Comment on attachment 797662 [details] [diff] [review]
Introduce js/TypeDecls.h, which holds very commonly used JS engine type declarations.

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

::: js/public/TypeDecls.h
@@ +16,5 @@
> +
> +#ifndef js_TypeDecls_h
> +#define js_TypeDecls_h
> +
> +#include <stdint.h>

Can you follow this by
#ifndef UINT32_MAX
# error "__STDC_LIMIT_MACROS must be defined before including this header"
#endif
?
Attachment #797662 - Flags: review?(luke) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e98958b5e50


> ::: js/public/TypeDecls.h
> @@ +16,5 @@
> > +
> > +#ifndef js_TypeDecls_h
> > +#define js_TypeDecls_h
> > +
> > +#include <stdint.h>
> 
> Can you follow this by
> #ifndef UINT32_MAX
> # error "__STDC_LIMIT_MACROS must be defined before including this header"
> #endif
> ?

I tried this but I got build failures on the B2G builds.  Since this file doesn't use UINT32_MAX or related constants, I just omitted it.  We've got bug 909576 open for that general problem.
This landed a lot of changes to Gecko that weren't reviewed by a relevant peer. I don't think we actually want those changes.
https://hg.mozilla.org/mozilla-central/rev/9e98958b5e50
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.