898263 - Kill off jsprvtd.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

I don't like the idea of having a catch-all header file with a bunch of random stuff in it.  Nor do I like exporting a header whose name is a shortening of "private".

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Move |jssrcnote| and |jsbytecode| out of jsprvtd.h.

This patch starts slimming down jsprvtd.h.

- It removes the unnecessary HashTable.h and Vector.h #includes.

- It removes the dead JS_BITS_PER_* and JS_GCTHING_* constants.

- It removes all the forward decls (except those used by non-forward decls
  later in jsprvtd.h, such as |class Shape| and |struct TypeObject|) and all
  the |typedef struct Foo Foo;| typedefs.  This required putting some forward
  decls in other files.

- It removes some now-unnecessary |#include "jsprvtd.h"| statements, including
  the two instances outside SpiderMonkey.  (jsprvtd.h cannot yet be removed
  from the EXPORTS list, though; that'll come a few patches further down this
  road.)

- Patch stats:

   28 files changed, 71 insertions(+), 159 deletions(-)

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Move |RegExpFlag| out of jsprvtd.h.

This patch moves RegExpFlag into vm/RegExpObject.h, which is a better place for
it.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Move |Env| out of jsprvtd.h.

This patch moves Env into vm/Debugger.h, which is a better place for it.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Move |JSTrapStatus| and related function typedefs out of jsprvtd.h.

This patch moves JSTrapStatus and all the debugger-related function typedefs
into jsdbgapi.h, which is a better place for them.

The only tricky part was that JSTrapStatus was used in jsfriendapi.h for the
CallContextDebugHandler() prototype.  But that function doesn't actually need
to be exported -- Gecko uses js_CallContextDebugHandler() instead -- so I just
removed it from the friend API.  I also moved CanCallContextDebugHandler() from
jsfriendapi.{h,cpp} into jsdbgapi.{h,cpp}, so it's next to
js_CallContextDebugHandler().

I also removed the |#include "jsdbgapi.h"| from nsJSEnvironment because it
was included *twice*.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Move some ClassExtension function typedefs out of jsprvtd.h.

jsapi.h is a much better spot for these things.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Move |jssrcnote| and |jsbytecode| out of jsprvtd.h.

This patch moves jssrcnote into SourceNotes.h, and creates a new exported
header jsbytecode.h for |jsbytecode|.  This is a precursor to un-exporting
jsprvtd.h, because exported headers jsfriendapi.h and jsdbgapi.h both use
|jsbytecode|.

The reason I gave it an old-style jsfoo.h filename is that it's exported, and
all the other exported headers also have old-style names.  I guess it could be
called frontend/Bytecode.h if you prefer.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Remove jsprvtd.h from EXPORTS.

This patch unexports jsprvtd.h.  The trick was to get jsclass.h (which is still
exported) to no longer depend on jsprvtd.h.

I had to convert a bunch of |js::PropertyDescriptor| types to
|JSPropertyDescriptor| in js/ipc/ and js/xpconnect/, which seems perfectly
reasonable.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Move the Handle* and Rooted* types out of jsprvtd.h.

Let's chuck these ones in a new header.

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 781521 [details] [diff] [review]
(part 8) - Move the Handle* and Rooted* types out of jsprvtd.h.

Terrence, you should take a look at this one too.

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: (part 10) - Move XDRMode from jsprvtd.h to jsatom.h.

This patch moves XDRMove into jsatom.h.  This isn't a great place for it, but I
didn't want to create a new file for it, and I tried moving it and XDRAtom()
into vm/Xdr.{h,cpp}, but it got complicated (cycles!) so I set my stun gun to
"pragmatic" :/

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: (part 11) - Move jsatomid from jsprvtd.h to ParseMaps.h.

This patch moves |jsatomid| from jsprvtd.h to ParseMaps.h.

This required the slightly dirty move of changing GET_UINT24 to use |unsigned|
instead of |jsatomid|.  But I figure it's ok, because GET_UINT16 already uses
|unsigned|.  There's nothing really tying GET_UINT24 to jsatomid anyway;  it's
used for a range of purposes.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: (part 12) - Move property-related typedefs from jsprvtd.h to ObjectImpl.h.

This'll do.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (part 13) - Move Native typedefs from jsprvtd.h to jsfun.h.

And this'll do, too.

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: (part 14) - Remove jsprvtd.h.

Hooray.

Comment 15

[Nicholas Nethercote [inactive]]

Attachment: (part 9) - Move IdValuePair from jsprvtd.h to ds/IdValuePair.h.

I forgot this patch.

Comment 16

[Nicholas Nethercote [inactive]]

Stats for all 14 patches:

 63 files changed, 386 insertions(+), 457 deletions(-)

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: (part 15) - Remove jsversion.h from EXPORTS.

While I'm here:  jsprototypes.h doesn't need jsversion.h, and with that gone,
jsversion.h need not be exported.

Comment 18

[Nicholas Nethercote [inactive]]

Attachment: (part 16) - Remove jsutil.h from EXPORTS.

With a little bit of effort, jsutil.h can be unexported, too.

Comment 19

[Nicholas Nethercote [inactive]]

Attachment: (part 17) - Remove jslock.h from EXPORTS.

Hey look, jslock.h also doesn't need to be exported.

Comment 20

[Terrence Cole [:terrence]]

Comment on attachment 781521 [details] [diff] [review]
(part 8) - Move the Handle* and Rooted* types out of jsprvtd.h.

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

Looks reasonable to me with the requested name change. There is some other stuff that happened to be in Rooting.h when we moved it to public that should probably move back to an internal header too.

r=me

::: js/src/gc/Handle.h
@@ +34,5 @@
> +typedef JS::Rooted<PropertyName*>      RootedPropertyName;
> +
> +} /* namespace js */
> +
> +#endif /* gc_Handle_h */

Please call this gc/Rooting.h to match js/public/RootingAPI.h and the name that we had previously.

Comment 21

[Nicholas Nethercote [inactive]]

Time to find a new reviewer.

Comment 22

[Jason Orendorff [:jorendorff]]

Comment on attachment 781477 [details] [diff] [review]
(part 1) - Move |jssrcnote| and |jsbytecode| out of jsprvtd.h.

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

This is a lot more than just moving jssrcnote and jsbytecode out of jsprvtd.h.

I think removing jsprvtd.h and instead scattering forward declarations everywhere is a bad idea, because repetition and boilerplate are bad. But making jsprvtd.h not exported is a huge win. Since this stack reduces total lines of code, I can live with it.

Comment 23

[Jason Orendorff [:jorendorff]]

Comment on attachment 781478 [details] [diff] [review]
(part 2) - Move |RegExpFlag| out of jsprvtd.h.

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

Stealing review. Yeah, this never belonged in jsprvtd.h to begin with.

Comment 24

[Jason Orendorff [:jorendorff]]

Comment on attachment 781481 [details] [diff] [review]
(part 5) - Move some ClassExtension function typedefs out of jsprvtd.h.

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

::: js/src/jsapi.h
@@ +959,5 @@
>  (* JSWeakmapKeyDelegateOp)(JSObject *obj);
>  
> +/* Signature for class initialization ops. */
> +typedef JSObject *
> +(* JSClassInitializerOp)(JSContext *cx, JS::HandleObject obj);

I don't think this should be public.

Comment 25

[Jason Orendorff [:jorendorff]]

Comment on attachment 781482 [details] [diff] [review]
(part 6) - Move |jssrcnote| and |jsbytecode| out of jsprvtd.h.

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

::: js/src/jsbytecode.h
@@ +8,5 @@
> +#define jsbytecode_h
> +
> +#include "mozilla/StandardInteger.h"
> +
> +typedef uint8_t jsbytecode;

Huh. Well, ok, I don't have a better idea.

Comment 26

[Jason Orendorff [:jorendorff]]

Comment on attachment 781483 [details] [diff] [review]
(part 7) - Remove jsprvtd.h from EXPORTS.

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

Righteous.

Is it more work than this to rename JSPropertyDescriptor to JS::PropertyDescriptor?

Comment 27

[Jason Orendorff [:jorendorff]]

Comment on attachment 781559 [details] [diff] [review]
(part 16) - Remove jsutil.h from EXPORTS.

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

::: js/xpconnect/src/XPCInlines.h
@@ +437,5 @@
>                                        XPCNativeInterface* iface) const
>  {
> +    int count = int(mInterfaceCount) < int(other->mInterfaceCount)
> +              ? int(mInterfaceCount)
> +              : int(other->mInterfaceCount);

Lame!

There's a Min template in mfbt/double-conversion/utils.h in case that helps. Not a big deal either way.

Comment 28

[Sean Stangl [:sstangl]]

My job here is done.

Comment 29

[Jeff Walden [:Waldo]]

(In reply to Jason Orendorff [:jorendorff] from comment #27)
> There's a Min template in mfbt/double-conversion/utils.h in case that helps.

Gecko uses std::min.  We should switch to that from our own hacked-up js::Min/Max at some point.

Comment 30

[Nicholas Nethercote [inactive]]

> (part 1) - Move |jssrcnote| and |jsbytecode| out of jsprvtd.h.
> 
> This is a lot more than just moving jssrcnote and jsbytecode out of
> jsprvtd.h.

True.  Not sure why I wrote that description.  I'll update before landing.

Thanks for the reviews!

Comment 31

[Nicholas Nethercote [inactive]]

> ::: js/src/jsapi.h
> @@ +959,5 @@
> >  (* JSWeakmapKeyDelegateOp)(JSObject *obj);
> >  
> > +/* Signature for class initialization ops. */
> > +typedef JSObject *
> > +(* JSClassInitializerOp)(JSContext *cx, JS::HandleObject obj);
> 
> I don't think this should be public.

Any suggestions where to put it?  It's used in jsapi.cpp and jsobj.cpp... put it in jsobj.h?

Comment 32

[Nicholas Nethercote [inactive]]

> Is it more work than this to rename JSPropertyDescriptor to
> JS::PropertyDescriptor?

Hmm, looking at DXR I think renaming it JS::PropertyDescriptor would be more work, so I'll punt on that.

Comment 33

[Nicholas Nethercote [inactive]]

> (part 15) - Remove jsversion.h from EXPORTS.
> 
> While I'm here:  jsprototypes.h doesn't need jsversion.h, and with that gone,
> jsversion.h need not be exported.

Actually, I won't do this.  Without this change, jsversion.h gets included (via jsprototypes.h) into jspubtd.h, which pretty much guarantees its seen by every .cpp file.  With this change, it's much less clear that this is true, and since jsversion.h is full of nothing but feature #defines we could get buggy behaviour if, for example, one file thinks JS_HAS_FOO is defined and another thinks it is not.

(We really need a better mechanism for the handful of files that should be seen by every .cpp files, such as jsversion.h, js-confdefs.h, and js-config.h.)

Comment 34

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 781559 [details] [diff] [review]
(part 16) - Remove jsutil.h from EXPORTS.

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

::: js/xpconnect/src/XPCInlines.h
@@ +437,5 @@
>                                        XPCNativeInterface* iface) const
>  {
> +    int count = int(mInterfaceCount) < int(other->mInterfaceCount)
> +              ? int(mInterfaceCount)
> +              : int(other->mInterfaceCount);

Gecko uses std::min/max

Comment 35

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab5587721464
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dcbea61c489
https://hg.mozilla.org/integration/mozilla-inbound/rev/f376300694fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/23451f2c9946
https://hg.mozilla.org/integration/mozilla-inbound/rev/46859f60fd63
https://hg.mozilla.org/integration/mozilla-inbound/rev/1546b85bbd8d
https://hg.mozilla.org/integration/mozilla-inbound/rev/737fb09c65a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/e27c1441baa1
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a78350b9749
https://hg.mozilla.org/integration/mozilla-inbound/rev/97bcd23d22b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b6354d92e6a
https://hg.mozilla.org/integration/mozilla-inbound/rev/78836964671f
https://hg.mozilla.org/integration/mozilla-inbound/rev/917941988708
https://hg.mozilla.org/integration/mozilla-inbound/rev/96f5e26e3e25
https://hg.mozilla.org/integration/mozilla-inbound/rev/7adfc69597d1

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ab5587721464
https://hg.mozilla.org/mozilla-central/rev/6dcbea61c489
https://hg.mozilla.org/mozilla-central/rev/f376300694fb
https://hg.mozilla.org/mozilla-central/rev/23451f2c9946
https://hg.mozilla.org/mozilla-central/rev/46859f60fd63
https://hg.mozilla.org/mozilla-central/rev/1546b85bbd8d
https://hg.mozilla.org/mozilla-central/rev/737fb09c65a3
https://hg.mozilla.org/mozilla-central/rev/e27c1441baa1
https://hg.mozilla.org/mozilla-central/rev/8a78350b9749
https://hg.mozilla.org/mozilla-central/rev/97bcd23d22b5
https://hg.mozilla.org/mozilla-central/rev/1b6354d92e6a
https://hg.mozilla.org/mozilla-central/rev/78836964671f
https://hg.mozilla.org/mozilla-central/rev/917941988708
https://hg.mozilla.org/mozilla-central/rev/96f5e26e3e25
https://hg.mozilla.org/mozilla-central/rev/7adfc69597d1
https://hg.mozilla.org/mozilla-central/rev/7d9d0f51250d