Some jsversion.h clean-ups

RESOLVED FIXED in mozilla26

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla26
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(3 attachments)

(Assignee)

Description

5 years ago
I have some patches that improve things about jsversion.h.
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 795228 [details] [diff] [review]
(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.
Attachment #795228 - Flags: review?(luke)
(Assignee)

Comment 3

5 years ago
Created attachment 795230 [details] [diff] [review]
(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.)
Attachment #795230 - Flags: review?(luke)
(Assignee)

Comment 4

5 years ago
Created attachment 795231 [details] [diff] [review]
(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.
Attachment #795231 - Flags: review?(luke)

Updated

5 years ago
Attachment #795228 - Flags: review?(luke) → review+

Updated

5 years ago
Attachment #795230 - Flags: review?(luke) → review+

Comment 6

5 years ago
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?
Attachment #795231 - Flags: review?(luke) → review+
(Assignee)

Comment 7

5 years ago
> 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.
(Assignee)

Comment 8

5 years ago
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?
Flags: needinfo?(jwalden+bmo)

Comment 9

5 years ago
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).
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 10

5 years ago
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.
(Assignee)

Comment 11

5 years ago
> - file a bug about Gecko's violation;

Bug 909576.
https://hg.mozilla.org/mozilla-central/rev/e3547f7fa0e4
https://hg.mozilla.org/mozilla-central/rev/96f58548cfd1
https://hg.mozilla.org/mozilla-central/rev/13233d9a4d9e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.