Closed Bug 898263 Opened 6 years ago Closed 6 years ago

Kill off jsprvtd.h

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: njn, Assigned: njn)

References

Details

(Whiteboard: [js:t])

Attachments

(17 files)

25.11 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
2.35 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
2.03 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
10.34 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
2.34 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
3.93 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
52.68 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
4.31 KB, patch
jorendorff
: review+
terrence
: feedback+
Details | Diff | Splinter Review
1.42 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
2.22 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.49 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.27 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
9.62 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
2.41 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.46 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
2.99 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.36 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
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".
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(-)
Attachment #781477 - Flags: review?(jorendorff)
This patch moves RegExpFlag into vm/RegExpObject.h, which is a better place for
it.
Attachment #781478 - Flags: review?(jorendorff)
This patch moves Env into vm/Debugger.h, which is a better place for it.
Attachment #781479 - Flags: review?(jorendorff)
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*.
Attachment #781480 - Flags: review?(jorendorff)
jsapi.h is a much better spot for these things.
Attachment #781481 - Flags: review?(jorendorff)
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.
Attachment #781482 - Flags: review?(jorendorff)
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.
Attachment #781483 - Flags: review?(jorendorff)
Let's chuck these ones in a new header.
Attachment #781521 - Flags: review?(jorendorff)
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.
Attachment #781521 - Flags: feedback?(terrence)
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" :/
Attachment #781523 - Flags: review?(jorendorff)
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.
Attachment #781526 - Flags: review?(jorendorff)
And this'll do, too.
Attachment #781532 - Flags: review?(jorendorff)
Hooray.
Attachment #781533 - Flags: review?(jorendorff)
I forgot this patch.
Attachment #781534 - Flags: review?(jorendorff)
Summary: Slim down and un-export jsprvtd.h → Kill off jsprvtd.h
Stats for all 14 patches:

 63 files changed, 386 insertions(+), 457 deletions(-)
While I'm here:  jsprototypes.h doesn't need jsversion.h, and with that gone,
jsversion.h need not be exported.
Attachment #781557 - Flags: review?(jorendorff)
With a little bit of effort, jsutil.h can be unexported, too.
Attachment #781559 - Flags: review?(jorendorff)
Hey look, jslock.h also doesn't need to be exported.
Attachment #781564 - Flags: review?(jorendorff)
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.
Attachment #781521 - Flags: feedback?(terrence) → feedback+
Time to find a new reviewer.
Attachment #781477 - Flags: review?(jorendorff) → review?(sstangl)
Attachment #781478 - Flags: review?(jorendorff) → review?(sstangl)
Attachment #781479 - Flags: review?(jorendorff) → review?(sstangl)
Attachment #781480 - Flags: review?(jorendorff) → review?(sstangl)
Attachment #781481 - Flags: review?(jorendorff) → review?(sstangl)
Attachment #781482 - Flags: review?(jorendorff) → review?(sstangl)
Attachment #781483 - Flags: review?(jorendorff) → review?(sstangl)
Attachment #781521 - Flags: review?(jorendorff) → review?(sstangl)
Attachment #781523 - Flags: review?(jorendorff) → review?(sstangl)
Attachment #781526 - Flags: review?(jorendorff) → review?(sstangl)
Attachment #781528 - Flags: review?(jorendorff) → review?(sstangl)
Attachment #781532 - Flags: review?(jorendorff) → review?(sstangl)
Attachment #781533 - Flags: review?(jorendorff) → review?(sstangl)
Attachment #781534 - Flags: review?(jorendorff) → review?(sstangl)
Attachment #781557 - Flags: review?(jorendorff) → review?(sstangl)
Attachment #781559 - Flags: review?(jorendorff) → review?(sstangl)
Attachment #781564 - Flags: review?(jorendorff) → review?(sstangl)
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.
Attachment #781477 - Flags: review?(sstangl) → review+
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.
Attachment #781478 - Flags: review?(sstangl) → review+
Attachment #781479 - Flags: review?(sstangl) → review+
Attachment #781480 - Flags: review?(sstangl) → review+
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.
Attachment #781481 - Flags: review?(sstangl) → review+
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.
Attachment #781482 - Flags: review?(sstangl) → review+
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?
Attachment #781483 - Flags: review?(sstangl) → review+
Attachment #781521 - Flags: review?(sstangl) → review+
Attachment #781534 - Flags: review?(sstangl) → review+
Attachment #781523 - Flags: review?(sstangl) → review+
Attachment #781526 - Flags: superreview+
Attachment #781526 - Flags: superreview+
Attachment #781526 - Flags: review?(sstangl)
Attachment #781526 - Flags: review+
Attachment #781528 - Flags: review?(sstangl) → review+
Attachment #781532 - Flags: review?(sstangl) → review+
Attachment #781533 - Flags: review?(sstangl) → review+
Attachment #781557 - Flags: review?(sstangl) → review+
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.
Attachment #781559 - Flags: review?(sstangl) → review+
Attachment #781564 - Flags: review?(sstangl) → review+
My job here is done.
(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.
> (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!
> ::: 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?
> 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.
> (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 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
You need to log in before you can comment on or make changes to this bug.