As seen in bug 599764, the use of NSPR fixed-width types in SpiderMonkey public API functions can be problematic, since it's hard to predict whether a given compilation unit will end up seeing the definitions from nsprpub/pr/include/obsolete/protypes.h or js/src/jsotypes.h. Since we go to the trouble of defining our JS{,U}int{8,16,32,64,Ptr} types ourselves, we should use them. The public API functions should be the first priority, since these can be affected by the protypes.h/jsotypes.h #include race, and can cause linkage problems due to the fact that C++ mangling can generate distinct names for integer types that have the same width and signedness. But in general, we should prefer our in-house definitions.

So if I understand this correctly, we want to eliminate all (direct) uses of the following types anywhere in JS code: float64; int16; int32; int64; int8; intn; uint16; uint32; uint64; uint8; uint; uintn; Is that right?

(In reply to comment #1) > So if I understand this correctly, we want to eliminate all (direct) uses of > the following types anywhere in JS code: If those are the types that come from either NSPR's protypes.h or jsotypes.h, then, yes.

jimb's handy chart: jsapi.h -> js-config.h -> jspubtd.h -> jstypes.h -> js-config.h* -> js-cpucfg.h -> jsinttypes.h -> js-config.h* -> jsotypes.h -> jscompat.h -> jstypes.h* -> jslong.h -> jstypes.h* -> jsval.h -> jsutil.h -> jstypes.h* -> jscustomallocator.h (not present) -> jsutil.h*

Why should we use the ugly JSUint8, etc. names? The uint8, etc., names can come unambiguously from our headers, or else. BTW, we own NSPR sources too. We can fix it to agree for all values. If the only conflict was uint64, that should be addressed directly -- not by lengthening and uglying up all the integer and floating point typenames. /be

The sad ancient (1998 or so) history whereby SpiderMonkey forked early NSPR-2 headers is just that: ancient history. We should prevail over the dead hand of the past. On the aesthetic front, and also to avoid drudgework renaming all over (not just us but big embedders like Bloomberg), I'm going to insist we stick with the short names. Anything else is making work for many in order to save some few (NSPR or JS, doesn't matter) a much smaller amount of work reconciling the uint64 types. /be

Brendan: the problem with the short names (uint8, etc.) is that they are also defined by some system header files or headers of other open source projects. NSPR's "obsolete/protypes.h" has some ugly ifdefs to deal with conflicts with system headers. I would use the C99 types uint8_t, etc. in new code.

(In reply to comment #4) > Why should we use the ugly JSUint8, etc. names? The uint8, etc., names can > come unambiguously from our headers, or else. Taking into account both brendan and wtc's comments, I think the best thing for long term lovability of the code is: - use C99 types (uint8_t etc) in public headers (listed in comment 3) - use shortened C99 types (uint8 etc) internally. Brendan, does this meet with your approval? Wan-Teh, do you see any technical problem with this?

As a sidenote, that's not only for Spidermonkey.. other parts of the tree do ugly things, see https://bugzilla.mozilla.org/show_bug.cgi?id=651444 for example.

This removes all uses of uint8 etc from public headers. There is more to do here, but this is a good first step, and we should probably file followups for the rest: - replace JSUint8 (etc) with uint8_t (etc) in public headers - remove jscompat.h - remove JSUint8 (etc) definitions (fixing the rest of the browser is outside the scope of things I want to even think about)

(In reply to comment #9) > Created attachment 539883 [details] [diff] [review] [review] > Switch to JSUint32 etc in public headers Let's not do this. JSUint32 is old cruft from NSPR forkage, as pointed out in comment 5, and we want it to go away.

OK, so I guess I'll go straight to step 2, in which JSUint32 etc are replaced with uint32_t etc.

Somewhat incredibly, visual studios prior to VS2010 do not support stdint.h [1]. So I don't think we can use uint32_t etc. I guess this means we should stick with JSUint32 etc, but would love to be corrected? [1] https://connect.microsoft.com/VisualStudio/feedback/details/99131/c99-header-stdint-h-missing

(In reply to comment #12) > Somewhat incredibly, visual studios prior to VS2010 do not support stdint.h > So I don't think we can use uint32_t etc. That's right, NSPR types are more portable than <stdint.h>'s ones. I guess I'm missing why NSPR types aren't eligible for SpiderMonkey.

(In reply to comment #13) > (In reply to comment #12) > > Somewhat incredibly, visual studios prior to VS2010 do not support stdint.h > > So I don't think we can use uint32_t etc. > > That's right, NSPR types are more portable than <stdint.h>'s ones. I guess > I'm missing why NSPR types aren't eligible for SpiderMonkey. See https://bugzilla.mozilla.org/show_bug.cgi?id=599764

(In reply to comment #12) > I guess this means we should > stick with JSUint32 etc, but would love to be corrected? That's silly of course. We can just supply our own, and use configure to check.

I've long thought it'd be a good idea to permit the user to provide a <stdint.h>-alike file to define fixed-size integer types. Then embedders with easily-accessible stdint.h could use that, and embedders without would use something of their own devices customized to their needs. SpiderMonkey could ship with a fallback stdint.h-alike if nothing were specified, with the caveat that its functionality would be trickier to use than just specifying the system-standard <stdint.h>. Of course, I could be missing some complexity here that makes this idea not quite so pleasing as it seems.

From what i understand, only Windows before VS2010 doesn't provide that C99 header ? Can't the header be added for that platform only ?

(In reply to comment #16) > SpiderMonkey could ship with a fallback stdint.h-alike if nothing were specified, (In reply to comment #17) > Can't the [<stdint.h>] be added for [Windows before VS2010] only ? Simulating standard types is fragile, because different libraries may do this differently: SpiderMonkey : typedef unsigned long uint32_t; 3rdPartyLib : typedef unsigned long long uint32_t; Application, uses both SpiderMonkey, 3rdPartyLib: uint32_t ... is what ???

My intention was to use an existing stdint.h library, probably one of: https://code.google.com/p/msinttypes/ http://www.azillionmonkeys.com/qed/pstdint.h http://www.boost.org/doc/libs/1_37_0/boost/cstdint.hpp (unlikely)

(In reply to comment #18) > SpiderMonkey : typedef unsigned long uint32_t; > 3rdPartyLib : typedef unsigned long long uint32_t; > > Application, uses both SpiderMonkey, 3rdPartyLib: uint32_t ... is what > ??? I think (I could be wrong here) that we should be letting the user provide stdint.h. In the case that the user does not, only then should we provide one. In the case of 3rdPartyLib, I believe the requirement should be on the user to provide a stdint.h to both libraries.

General guidance: - Must work with minimal developer effort for Firefox builds on all platforms. For Windows, at this time, that means MSVC 8 (for official builds; == Visual C++ 2005), 9, and 10. - Must work with minimal developer effort for Spidermonkey shell builds on all platforms. For Windows, this includes MSVC 8, 9, and 10. - Need to support embedders--we can discover what they need as time goes and fix things, but trying system header, our default header, user-provided header in turn seems like a good sequence.

Although I filed this bug (because I thought Luke was asking me to do so in bug 599764), I'm not sure this should be our first choice of investment, if you catch my drift. When C++ linkage problems arise --- which isn't that often --- simply changing NSPR to match the system's <stdint.h> types seems like a fine way to resolve them. I don't agree that letting the embedder specify a stdint.h work-alike is a good idea. That is not a knob anyone is looking forward to tweaking. When someone's having an issue (as with OpenBSD in bug 599764), we should smooth it out so that everything Just Works. (In reply to comment #5) > The sad ancient (1998 or so) history whereby SpiderMonkey forked early > NSPR-2 headers is just that: ancient history. We should prevail over the > dead hand of the past. What is the best change to clear out the wrinkles caused by this particular episode?

I can't make a value judgment as to whether this is a worthwhile investment, but, in the two years I have been here, the general "these types suck, can we fix them?" issue has come up (leading to involved discussions like we are having here) at least three times and is expressed in passing at least monthly. I know this is just part of life with long-lived software and we can't fix everything in this category but something cool has been happening in SpiderMonkey for the last couple years: lots of long-standing annoyances and complications which one may have assumed would always be with us have been incrementally improved and while no one change seems like a great investment the aggregate effect is that our codebase as a whole feels an order of magnitude better. I'm not saying we definitely should fix this -- just that I'd rather not steer Paul away from it because I believe that its fixes like this that allow SpiderMonkey to avoid grinding to a halt under the weight of its own naturally-accruing cruft.

I understand that Brendan holds a veto, but it would still be interesting to know if anyone besides Brendan prefers the present int32-style types to the standard (since 1999) int32_t-style types. Requiring embedders to drop use of the JSInt32-style types would be very unfriendly, for little benefit. SpiderMonkey has attractive competition, so we must be considerate. However, that doesn't mean we have to use those types in our API or our code --- just that we have to define them in jsapi.h. To me, the ideal solution would be to use the <stdint.h> int32_t-style types throughout, with a backstop for older MSVC versions, and define the JSInt32-style types in jsapi.h. To Brendan, I gather the ideal solution would be to use the NSPR int32-style types throughout, and define the JSInt32-style types in jsapi.h, but not use them internally. In either case, jsotypes.h should go, since we have control over the NSPR headers now.

(In reply to comment #24) > I understand that Brendan holds a veto, but it would still be interesting to > know if anyone besides Brendan prefers the present int32-style types to the > standard (since 1999) int32_t-style types. I prefer not having to press the shift key so I don't like the _t types or the BIGname types which have confusing capitalization. But it sounds like this bug is just for the API and not in our internal code. (Also, the competition uses standard types in their API which seems more friendly than weird non-standard ones.)

> (In reply to comment #24) > I prefer not having to press the shift key so I don't like the _t types or > the BIGname types which have confusing capitalization. Okay. Good to know. > But it sounds like > this bug is just for the API and not in our internal code. (Also, the > competition uses standard types in their API which seems more friendly than > weird non-standard ones.) We should use a single typedef family both internally and in the API. Having two (or three) families in use is really the most serious injury to correct. If that type family is not JSInt32's, then we should define JSInt32's explicitly in jsapi.h (or something #included from it) in terms of what we do use. But there's no reason to keep that family in more widespread use than that if we're not using it internally.

(In reply to comment #20) > (In reply to comment #18) > > Simulating standard types is fragile, ... > > > > SpiderMonkey : typedef unsigned int uint32_t; > > 3rdPartyLib : typedef unsigned long uint32_t; > > > > Application, uses both SpiderMonkey, 3rdPartyLib: uint32_t ... is what ??? > > I think (I could be wrong here) that we should be letting the user provide > stdint.h. In the case that the user does not, only then should we provide > one. In the case of 3rdPartyLib, I believe the requirement should be on the > user to provide a stdint.h to both libraries. For this to work, every particular 3rdPartyLib shall follow your proposal. This is not realistic. I'm talking this because of sad experience. Even if all parties libraries follow agreement, the troubles may still arise, because of clustering around types: libs A, B: uint32_t ::= unsigned int C, D: uint32_t ::= unsigned long Assume applications were happy using either {A,B} or {C,D}, but some day some application needs {A,B,C}. Should user recompile {C,D} and all dependencies ? A consistent solution is to define your own types within controlled boundaries. JSInt32 or smth. else. When a compiler will match XUInt32 and YUint32 in the application (user) code, this won't be a problem, even if XUInt32 is 'int' and YUint32 is 'long'.

Can we back up and make sure we're clear on exactly what problems we're trying to solve here? IIUC, there are 2 problems with the current situation: P1. Types like |uint32| are defined in both jsotypes.h and in nsprpub/pr/include/obsolete/protypes.h. Thus, cpp files can end up seeing different definitions in Firefox builds (and other Gecko-based things). P2. Types like |uint32| can collide with types defined in other library headers. Thus, cpp files used by embedders can see different definitions. Are there more? To me, neither problem is particularly pressing, so it doesn't make sense to take a solution that's even a little bit bad. P1, though, seems like it can be solved directly by deleting one of the duplicate headers, or alternatively by making them match exactly, although that seems second best. Either way, as long as it works, it doesn't gross up Spidermonkey code. I also wonder about "obsolete/prtypes.h". What does the "obsolete" mean, exactly? Does it mean NSPR shouldn't even have that header any more? Or that it shouldn't be used in Gecko? P2 is harder, because it involves changing the names. And I don't think we should do that before everyone who cares gets a chance to express their opinion. Personally, I find "JSUint32" to be unspeakably vile. And I do like the current names--short and cruftless. Next best for me would be uint32_t; IME following standards is generally a good bet. The next best thing after that that immediately comes to mind is jsuint32--has the fake namespacing, no ugly caps. Using the standard types doesn't directly work on MSVC prior to version 10 (aka 2010). The easiest solution there would be to ship the header (or a close derivative) that comes with MSVC 10, and make the build system use it iff the user is building with MSVC8/9. The license on that file is: /* * Copyright (c) 1992-2009 by P.J. Plauger. ALL RIGHTS RESERVED. * Consult your license regarding permissions and restrictions. V5.20:0009 */ I don't know exactly what that means but we can find out. Konstantin points out that this still leaves a potential problem for embedders: what do they do if they don't have a stdint.h and also want to integrate with a library that provides different definitions of int32_t? (Is that the only problem scenario?) On that issue, I'd like to know: (a) how much does that actually happen? (is it happening now, etc?), (b) would shipping a substitute stdint.h make anyone worse off than they are now, and (c) given that the problem presumably does exist now, how are people dealing with it?

(In reply to comment #28) One more: P3. Get rid of the 'unspeakably vile' types. Judging from your comment, I think we agree on both the goals and the way to deal with them. The only thing that's slightly different in my plan than what you discuss is that I feel we should use the stdint.h types in the public headers. I want to use uint32 internally (I think everyone likes this) rather than uint32_t, and the plan is to typedef one to the other in internal headers. With regard to the problem Konstantin describes, I don't think this is a difficult issue. We need to provide a stdint.h only when one is not provided. This would be implemented as a configure check for stdint.h (actually, we already have one). If an embedder disagrees with either the system definition or our definition of stdint.h, they need merely provide their own using standard unix-y idioms. Our internal types would derive from stdint.h types, so our internal uint32 would be identical to uint32_t and embedder::uint32_t, avoiding linkage problems. Konstantin, please correct me if I've misunderstood what you're saying.

(In reply to comment #29) > (In reply to comment #28) > > One more: > P3. Get rid of the 'unspeakably vile' types. That's an action, not a problem. > Judging from your comment, I think we agree on both the goals and the way to > deal with them. I haven't expressed a full opinion on a solution--I would need more information about the team's naming preferences and embedder needs. > The only thing that's slightly different in my plan than > what you discuss is that I feel we should use the stdint.h types in the > public headers. I want to use uint32 internally (I think everyone likes > this) rather than uint32_t, and the plan is to typedef one to the other in > internal headers. This is OK *if* we can fix all the problems relating to using stdint.h with MSVC8/9, etc., to the point that everything Just Works.

Current Windows thorniness issues aside (alternately, assuming they could be satisfactorily resolved): I prefer uint32_t to uint32 on the grounds that we shouldn't be reinventing, or even naming, our own thing when we can help it, and it's not that much extra typing, to win with consistency with others. That includes newer Mozilla code like WOFF support: http://hg.mozilla.org/mozilla-central/annotate/cd95d565c4d9/gfx/thebes/woff.h#l43 Seems to me if others are sucking it up (or something), we should too. Now back to other bugwork...

Previous and current module owner say "uint32 not uint32_t" -- can we move on this and then get on to more crucial bugs? /be

To restate the specific problem I asked jimb to file about: jsval had to be typedef'd to JSUint64 because uint64 wasn't good enough -- that's not right. I think our problem solving algorithm should be: if (we can make uint64 work) switch jsval back to uint64 else make uint64_t work switch public headers to use _t types kill all JS* int types It sounds like to get the first condition true we have to change NSPR headers; can we do that?

(In reply to comment #29) > With regard to the problem [conflicting simulated standard types], > I don't think this is a difficult issue. We need to provide a stdint.h > only when one is not provided. ... > > If an embedder disagrees with either the system definition or our definition > of stdint.h, they need merely provide their own using standard unix-y idioms. > > Konstantin, please correct me if I've misunderstood what you're saying. If system owner substitutes it's own global <stdint.h>, he turns system to having <stdint.h>. No problem. But let take a platform that lacks <stdint.h> (Win+VS?) Let imagine, that every independent library provides it's own simulations of standard types ( uint32_t, ... ) for this platform. It would be a nightmare to build any application, that uses more than one library. Therefore, polite library won't simulate standard types, although this is path of least effort. When in Rome ... By the way, why not just: #if available "stdint.h" jsi32 ::= int32_t, jsu32 ::= uint32_t #else jsi32 ::= PRInt32, jsu32 ::= PRUInt32 #endif The other honest decision is to drop support for platforms, that lack <stdint.h>.

(In reply to comment #33) > To restate the specific problem I asked jimb to file about: jsval had to be > typedef'd to JSUint64 because uint64 wasn't good enough -- that's not right. Right. > It sounds like to get the first condition true we have to change NSPR > headers; can we do that? NSPR is just broken. Everyone shipping it is patching it. See: https://bugzilla.mozilla.org/show_bug.cgi?id=290725

(In reply to comment #35) > NSPR is just broken. Everyone shipping it is patching it. Hmm... :) Disagree. NSPR is a robust and strong software :) > See: https://bugzilla.mozilla.org/show_bug.cgi?id=290725 This particular bug is about choosing default build bitness (32/64) for 64-bit platforms. Not very terrific. If you consider PR.Int.. types insufficiently portable, please, argue.

I feel obliged to comment on this bug as its filer, but I don't mean to exaggerate its importance by continuing the conversation. ("So don't!") As far as I know, there are no pressing problems in this area at the moment, now that bug 599764 is landed. I think the problem dmandelin missed in comment 28 is: P3. SpiderMonkey has three vocabularies of fixed-width types in widespread use. Third-party code uses the <stdint.h> types; SM itself uses a mix of the NSPR and JS* types. This is distracting. I chose the word "Distracting" deliberately. The "problem" here is in the class Luke identified in comment 23: the issue comes up in conversation from time to time, and the present arrangement is not what anyone would have chosen from the outset, but rather a consequence of history, and the general efforts to simplify such things have made the code base much easier to work in of late. "NSPR sucks" is not the topic of this bug, and is *even worse* to take on here than that of comment 0, which was arguably not great to begin with.

(In reply to comment #37) > As far as I know, there are no pressing problems in this area at the moment, > now that bug 599764 is landed. I agree nothing is "pressing" here; nothing is burning (we know of :). But, to reiterate comment 33, something more than stylistically wrong. AFAIC, a minimal resolution of this problem would be us not having the dual stances: "don't use JS* int types anywhere, they are gross and old" and "you might need to use JSUint64 sometimes to avoid build errors somewhere".

This issue just wasted another hour of me setting up a windows build to figure out the linker error that got me backed out of mozilla-inbound.

(Apparently even uint32 can lead to linker errors (unsigned int vs. unsigned long))

(In reply to comment #40) > (Apparently even uint32 can lead to linker errors (unsigned int vs. unsigned > long)) I hit this same issue in Bug 650519, around comment 56. When I poked around at it before, the problem was something like that the build system tries a few different options to define uint32, in fairly arbitrary order, and NSPR tries things in a different order than whatever the source of the JS version is. So when you include a header in browser land, you get uint32 = unsigned int, and in JS land you get uint32 = unsigned long. Or the other way around, it has been a while. I think this particular issue could probably be fixed just by poking around in whatever build file picks the definition of uint32. (Of course, I think my solution that worked around this problem was ultimately a better one, but no matter...)

Not 100% sure, but i've hit a build error probably linked to that issue in Bug 676924.

dmandelin, waldo: I think this change has been done elsewhere and so this bug can be closed?