Last Comment Bug 711672 - Break most of the mfbt->JS header dependence
: Break most of the mfbt->JS header dependence
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: MFBT (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-16 18:22 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-12-19 09:35 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove DEFINE_LOCAL_CLASS_OF_STATIC_FUNCTION (1.32 KB, patch)
2011-12-16 18:24 PST, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review
Move the JS macros into mfbt (11.46 KB, patch)
2011-12-16 18:29 PST, Jeff Walden [:Waldo] (remove +bmo to email)
cjones.bugs: review+
luke: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-16 18:22:59 PST
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] (remove +bmo to email) 2011-12-16 18:24:49 PST
Created attachment 582454 [details] [diff] [review]
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] (remove +bmo to email) 2011-12-16 18:29:02 PST
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.  :-)
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-16 18:33:48 PST
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] 2011-12-17 13:24:28 PST
Comment on attachment 582454 [details] [diff] [review]
Remove DEFINE_LOCAL_CLASS_OF_STATIC_FUNCTION

Hah.
Comment 5 :Ms2ger 2011-12-17 13:34:17 PST
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] (remove +bmo to email) 2011-12-17 13:39:25 PST
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] 2011-12-17 13:42:28 PST
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] (remove +bmo to email) 2011-12-17 13:52:57 PST
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] (remove +bmo to email) 2011-12-18 12:43:14 PST
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 2011-12-18 15:48:15 PST
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] (remove +bmo to email) 2011-12-18 20:39:27 PST
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 13 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-19 09:35:57 PST
(In reply to Luke Wagner [:luke] from comment #7)
> Feel free to file as followup or ignore, but[...]

Bug 712034 filed.

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