909171 - Some jsversion.h clean-ups

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

I have some patches that improve things about jsversion.h.

Comment 1

[Nicholas Nethercote [inactive]]

Preliminary info...

js-config.h and jsversion.h should be included everywhere in SpiderMonkey.

js-config.h is currently included by jstypes.h and jsapi.h.  I think jstypes.h
alone gives total coverage (because jsapi.h includes jstypes.h), so that seems
ok.

However, jsversion is currently included in jspubtd.h and several other less
widely-included files.  And I know for certain that jspubtd.h is *not* included
everywhere -- there are ~20 .h files and ~20 .cpp files that don't include it.
So this needs fixing.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Add JS_ prefixes to macros missing them in jsversion.h.

This patch prefixes some constants in jsversion.h with "JS_".  These constants
are externally visible, so it seems worthwhile to avoid polluting the global
namespace with non-prefixed constants.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove JS_VERSION.

This patch removes the JS_VERSION constant, which is deprecated, and not used
anywhere in Gecko.  (Fun fact:  |JS_VERSION| is also defined in
linux/joystick.h;  the "JS" there is, of course, short for "joystick".)

It works for Gecko, but there may be other embedding considerations that I'm
unaware of.

(BTW, There's a JS_VERSION const in addon-sdk/source/lib/sdk/content/worker.js;
it's unused and I don't know if it has any meaning, so I left it alone.)

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Fix up jsversion.h includes.

This patch:

- Includes jsversion.h from only jstypes.h, which is enough (and actually
  improves coverage relative to the current).

- Removes the js-config.h include from jsapi.h, because the one in jstypes.h
  subsumes it.

- Tweaks some #ifndef guard names to be consistent with everywhere else.

Comment 5

[Nicholas Nethercote [inactive]]

Good-looking try run:
https://tbpl.mozilla.org/?tree=Try&rev=a26eee8f0877

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 795231 [details] [diff] [review]
(part 2) - Fix up jsversion.h includes.

I wonder if we could move jstypes.h to js/public and give it a name indicating "this file is included transitively by every js header" because "jstypes.h" (or "Types.h" in the public/ naming scheme) isn't a great name.  Prerequisites.h?

Comment 7

[Nicholas Nethercote [inactive]]

> Prerequisites.h?

That's an excellent question.  Turns out we already have something like this, called js/RequiredDefines.h.  I tried including jsversion.h and js-config.h from that file and I swear it didn't work, but I discussed it with Waldo and am trying it again and now it seems like it is working.

Comment 8

[Nicholas Nethercote [inactive]]

Ah, now I remember.  The problem is that js-confdefs.h gets force-included for the .cpp files within js/src/.  However, Gecko .cpp files that include JS headers don't get js-confdefs.h.  If I try including js-config.h and jsversion.h only within RequiredDefines.h, I get this error:

In file included from /home/njn/moz/mi6/xpcom/glue/nsMemory.cpp:9:
In file included from /home/njn/moz/mi6/xpcom/glue/../build/nsXPCOMPrivate.h:13:
In file included from ../../dist/include/xptcall.h:15:
In file included from ../../dist/include/js/Value.h:19:
In file included from ../../dist/include/js/RootingAPI.h:13:
In file included from ../../dist/include/jspubtd.h:17:
In file included from ../../dist/include/jstypes.h:149:
../../dist/include/jscpucfg.h:106:3: error: "Cannot determine endianness of your
 platform. Please add support to jscpucfg.h."

The problem is that jscpucfg.h needs js-config.h to be included first, so that JS_HAVE_ENDIAN_H is defined.

So... is RequiredDefines.h being included as widely within Gecko as it should be?  It has this comment:

"Embedders should add this file to the start of the command line via -include or a similar mechanism, or SpiderMonkey public headers may not work correctly."

and I think Gecko currently violates this.  What do you think, Waldo?

Comment 9

[Jeff Walden [:Waldo]]

Gecko does indeed currently violate it.  But it gets away with this by having a similar mechanism, mozilla-config.h(.in), that does everything that js/RequiredDefines.h does at present.  This is pretty un-kosher; possibly having one mfbt header that does the same thing, that gets included first before anything else in all of Gecko *and* SpiderMonkey, might be a solution.

That said, this is kind of on the cosmetic side as far as problems go, so I haven't been exercised enough to try to un-duplicate stuff, not when we're only talking one #define right now (three, I think, if I ever manage to dust off and land bug 730805).

Comment 10

[Nicholas Nethercote [inactive]]

Luke, here's what I'll do:
- file a bug about Gecko's violation;
- augment the "jstypes.h is (or should be!) included by every file in SpiderMonkey" comment in part 2 to point to that bug.
- land these patches.

Comment 11

[Nicholas Nethercote [inactive]]

> - file a bug about Gecko's violation;

Bug 909576.

Comment 12

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3547f7fa0e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/96f58548cfd1
https://hg.mozilla.org/integration/mozilla-inbound/rev/13233d9a4d9e

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e3547f7fa0e4
https://hg.mozilla.org/mozilla-central/rev/96f58548cfd1
https://hg.mozilla.org/mozilla-central/rev/13233d9a4d9e