Break most of the mfbt->JS header dependence

RESOLVED FIXED in mozilla11



6 years ago
6 years ago


(Reporter: Waldo, Assigned: Waldo)



Firefox Tracking Flags

(Not tracked)



(2 attachments)

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/ )
Created attachment 582454 [details] [diff] [review]

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.
Attachment #582454 - Flags: review?(luke)
Created attachment 582455 [details] [diff] [review]
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.  :-)
Attachment #582455 - Flags: review?(luke)
Attachment #582455 - Flags: review?(jones.chris.g)
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

6 years ago
Comment on attachment 582454 [details] [diff] [review]

Attachment #582454 - Flags: review?(luke) → review+
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?
Hmm, yeah, that should have been |extern MOZ_EXPORT_API(int)|.

If you meant about the method name:

Comment 7

6 years ago
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...).
Attachment #582455 - Flags: review?(luke) → review+
I think the only dependency now is JS_Assert.

I'll file a followup for the other type stuff, good call.
Component: General → MFBT
QA Contact: general → mfbt for the first patch -- waiting on the other r? for the second half.
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.
Attachment #582455 - Flags: review?(jones.chris.g) → review+
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.

And that's everything here, should be good to close with the next m-i->m-c merge.
Target Milestone: --- → mozilla11
Last Resolved: 6 years ago
Resolution: --- → FIXED
(In reply to Luke Wagner [:luke] from comment #7)
> Feel free to file as followup or ignore, but[...]

Bug 712034 filed.
You need to log in before you can comment on or make changes to this bug.