711672 - Break most of the mfbt->JS header dependence

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jeff Walden [:Waldo]]

jstypes.h exposes a lot of stuff that definitely shouldn't be in mfbt.  Trim that down to the minimal bit, including with respect to exposed macros.  (This was also a convenient way to learn a bit about how all these macros work, and what the differences among them all were.  I think I can go about fixing the mass of Windows warnings compiling the JSAPI tests with what I know now!  \o/ )

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Remove DEFINE_LOCAL_CLASS_OF_STATIC_FUNCTION

This macro is randomly in the middle of all the JS macros.  It's totally unused, and it's not prefixed to indicate that it was some sort of stable API, so we should remove it.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Move the JS macros into mfbt

I am highly ambivalent about having MOZ_BEGIN_EXTERN_C and MOZ_END_EXTERN_C.  I find it much clearer to simply have #ifdef __cplusplus / extern "C" / #endif (or less, when in code that's definitely C++) and such.  I wouldn't add them if it were up to me.  I know at least a couple JS engine hackers think them unhelpfully obfuscatory.  But they're already there now, so this is just preserving status quo.

The JS engine should just use the MOZ_* macros directly, but that can and should be a separate patch that doesn't eat up time of anyone but JS hackers.  :-)

Comment 3

[Jeff Walden [:Waldo]]

Oh, oops, forgot to copy-paste this text out of a temporary textarea used for storage:

I'm pretty sure the __MWERKS__ bits are dead code now, and there are probably other little bits in there that could be trimmed, but I figured better to do the bare minimum to move first, only then doing whatever pruning is deemed desirable.

Note that |JSIntn| is a typedef to |int|, so there is no change to JS_Assert's signature.  mfbt obviously can't depend on JSIntn, JS_Assert's signature can't change, and I can't see why it makes sense for mfbt to provide a typedef for |int|, so the natural solution is to make the argument type simply |int|.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 582454 [details] [diff] [review]
Remove DEFINE_LOCAL_CLASS_OF_STATIC_FUNCTION

Hah.

Comment 5

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

Comment on attachment 582455 [details] [diff] [review]
Move the JS macros into mfbt

>--- a/mfbt/Types.h
>+++ b/mfbt/Types.h

>+ *   MOZ_EXTERN_API(int) MostRandomNumber(void);

Er, really?

Comment 6

[Jeff Walden [:Waldo]]

Hmm, yeah, that should have been |extern MOZ_EXPORT_API(int)|.

If you meant about the method name:

http://scienceblogs.com/cognitivedaily/2007/02/is_17_the_most_random_number.php

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 582455 [details] [diff] [review]
Move the JS macros into mfbt

Good change.  IIUC, this means zero dependencies from mfbt on js/*?  Man I'm glad you fixed the stdint situation!

Feel free to file as followup or ignore, but: it would be nice if jstypes.h were mv'd to js/public.  mfbt/Types.h will conflict if you just name it js/public/Types.h.  Looking at the remaining content of jstypes.h, there isn't too much, and what there is isn't types.  Perhaps it can just be injected into js/public/Utility.h?  Also there are a lot remaining goofballs to be moved into LegacyIntTypes.h (JSWord, JSUWord, JSPtrDiff, JSSize, JSInt, JSIntn...).

Comment 8

[Jeff Walden [:Waldo]]

I think the only dependency now is JS_Assert.

I'll file a followup for the other type stuff, good call.

Comment 9

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8184aa67241 for the first patch -- waiting on the other r? for the second half.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 582455 [details] [diff] [review]
Move the JS macros into mfbt

I very much hope that JSIntN is always guaranteed to be a synonym for int, or else this changes the ABI.  But I expect that to be true.

Comment 11

[Jeff Walden [:Waldo]]

Changing JSIntn would change ABI; it seems unlikely to me that we'd change that typedef.  Nonetheless, I added a static assertion that JSIntn is int when pushing.

https://hg.mozilla.org/integration/mozilla-inbound/rev/38a35f0db9ff

And that's everything here, should be good to close with the next m-i->m-c merge.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/a8184aa67241
https://hg.mozilla.org/mozilla-central/rev/38a35f0db9ff

Comment 13

[Jeff Walden [:Waldo]]

(In reply to Luke Wagner [:luke] from comment #7)
> Feel free to file as followup or ignore, but[...]

Bug 712034 filed.