See bug 550610 for rationale. It seems prudent to nick the build sorcery required for this from jsstdint.h.

To be clear, this would be "Replace NSPR types with stdint types"?

Yes, "Replace not-stdint types with stdint types".

I would like this to happen. My reasons: 1) having fewer types means fewer template specializations, which means smaller executable code 2) for templates that have to be specialized for each integer types, such as CheckedInt's internal helpers, this would reduce source code complexity 3) for code that we share with other projects like WebKit, having custom PR types for no good reason is harmful, makes sharing code more difficult than necessary for no benefit. Again, CheckedInt is such an example.

(In reply to Benoit Jacob [:bjacob] from comment #3) > 1) having fewer types means fewer template specializations, which means > smaller executable code > 2) for templates that have to be specialized for each integer types, such > as CheckedInt's internal helpers, this would reduce source code complexity Both of these are only true in cases when, say, uint32_t is typedef'd to something different from what PRUint32 is. We saw that happen on windows for one type, but otherwise that hasn't been the case. > 3) for code that we share with other projects like WebKit, having custom PR > types for no good reason is harmful, makes sharing code more difficult than > necessary for no benefit. Again, CheckedInt is such an example. +1. Similarly, developers new to Mozilla are quite likely to have stdint experience, and have a ~0% chance of NSPR experience.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4) > (In reply to Benoit Jacob [:bjacob] from comment #3) > > 1) having fewer types means fewer template specializations, which means > > smaller executable code > > 2) for templates that have to be specialized for each integer types, such > > as CheckedInt's internal helpers, this would reduce source code complexity > > Both of these are only true in cases when, say, uint32_t is typedef'd to > something different from what PRUint32 is. We saw that happen on windows > for one type, but otherwise that hasn't been the case. Well, currently PR types might or might not be equal to std types. Code like CheckedInt should not be written in a way that makes assumptions in this respect, as that would place an undue burden on anyone wanting to tweak PR types in the future. So from the point of view of CheckedInt, we can't make any assumptions at all and we have to pay a high price even if it's not strictly needed with the current state of PR types.

Here I was having only 2) in mind. You're entirely right about 1).

FWIW, I think we could actually use stdint.h after we switch trunk to MSVC 2010 and drop support for earlier versions of the MSVC (the latter being doable sometime early in 2012, I imagine).

A wrinkle here is compatibility with embedders, who are likely to have their own stdint hacks that aren't compatible with Mozilla's. js/src/ dances and elaborate dance to not expose stdint types through "public" headers, while using them internally. If we were to switch to stdint types in XPCOM headers, then we'd have this problem in gecko too. Possibly worse. In mfbt/, we punted this problem by using "almost" stdint types, aka protypes, e.g. uint32 instead of uint32_t. These are safe for internal and public headers (at least as experience with js/src proves). We'll need to decide which we want in the long run. I lean towards having one set of types for internal/external, because that's not something I want to think about when writing code.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #8) We have talked about having: // jsstdint.h #ifdef JS_NON_STANDARD_INT_H # include JS_NON_STANDARD_INT_H #else # include <stdint.h> #endif allowing embeddings to override stdint.h. Of course, I might be missing a constraint in this system. In general, I would reallly like this to be fixed (cf. bug 662852). For some arcane reason, even uint32/64/etc have linker-based problems that periodically cause bustage! Thus, right now in SM, the *only* truly safe thing to use in public headers is the universally-agreed-to-be-ugly JSUint32/JSUint64/etc typedefs (which we do inconsistently). I would be very happy to use stdint.h in public JS headers and nuke the JSUint32 family from orbit. It would certainly give a clear answer for what int types to use in mfbt/ and drop the dependence of mfbt/ on jstypes.h.

Alright. Let's give it a shot. We don't need to wait on dropping support for older MSVC if we do something like #if defined(NON_STANDARD_INT_H) # include NON_STANDARD_INT_H #elif !(HAVE_STDINT_H) # include [what's now jsstdint.h] #else # include <stdint.h> #endif

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4) > Both of these are only true in cases when, say, uint32_t is typedef'd to > something different from what PRUint32 is. We saw that happen on windows > for one type, but otherwise that hasn't been the case. What types had such a mismatch between the NSPR PR* type and the analogous *_t type? Necko and PSM interface directly with NSPR and NSS, so this would be helpful to know. Also, I would like to change NSPR so that all the NSPR PR* types are typedef'd to the *_t types whenever stdint.h is available, to make NSPR's build system simpler and its IDE support (VS2010+ in particular) better.

We have a separate import of NSPR through ipc/chromium. IIRC, one version had |typedef long int32|, and the other had |typedef int int32|, which caused linkage problems depending on which NSPR header the decl/defn saw first. The change you propose to NSPR sounds good, but I'm not sure if it's worth the effort. Doesn't sound like too much work I guess.

(In reply to Brian Smith (:bsmith) from comment #11) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #4) > > Both of these are only true in cases when, say, uint32_t is typedef'd to > > something different from what PRUint32 is. We saw that happen on windows > > for one type, but otherwise that hasn't been the case. > > What types had such a mismatch between the NSPR PR* type and the analogous > *_t type? See bug #634793 and all the bugs blocked by it. I've had enough build failures because of PRInt64 != int64_t (and its unsigned variants).. Necko and PSM interface directly with NSPR and NSS, so this would > be helpful to know. Also, I would like to change NSPR so that all the NSPR > PR* types are typedef'd to the *_t types whenever stdint.h is available, to > make NSPR's build system simpler and its IDE support (VS2010+ in particular) > better.

(In reply to Brian Smith (:bsmith) from comment #11) > What types had such a mismatch between the NSPR PR* type and the analogous > *_t type? On Windows {u,}int32_t doesn't align with PR{Ui,I}nt32 -- one's {unsigned, }long, the other's {unsigned, }int. I have a vague recollection of having to Schlemiel-the-painter my way through some mismatch on Linux as well. I think it was {u,}int64_t and PR{Ui,I}nt64 not agreeing, although I don't remember with absolute certainty. But that'd match with the 64-bit-type changes in this patch from bug 708735, certainly (changes I know I made out of necessity): https://hg.mozilla.org/mozilla-central/rev/d6d732ef5650

The bulk of what needs to land here is auto-generated by this script, which I created based on the one I used in bug 690892. I can also attach a sample patch generated by it but it's huge (~21MB) and I don't think there is much point in reviewing the patch itself.

This patch makes the IDL parser generate and consume stdint types. Note that the stdint types will no longer be considered built-in types, and they're resolved using the typedefs in http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsrootidl.idl which gets rewritten by the script in Part 1.

We no longer need NSPR types in the IPDL parser to be considered as built-in types, since the IPDL files are rewritten by the script in Part 1.

I tried to be conservative in the auto-conversion script, and that left out a number of mentions of the NSPR types in the code base which I converted over manually after inspection. This patch should be fairly uninteresting but I'm asking for review on it in case I have made a mistake somewhere.

This patch adds #includes for StandardInteger.h where it was missing before (it is #included in nscore.h so it's available in most places in the code base). Wherever possible, I tried to remove the prtypes.h #include, but we're still using some things from that header so removing it entirely is not possible yet.

Comment on attachment 650575 [details] [diff] [review] Part 2: Make the IDL parser aware of stdint types Bug 780203 ruined my work here. I'll give this another shot and see what I need to do here.

Comment on attachment 650575 [details] [diff] [review] Part 2: Make the IDL parser aware of stdint types Actually this is still good!

This script moves comm-central over to stdint types as well. Let there be a race on who reviews this sooner!

> convert PRIntn int32_t > convert PRUintn uint32_t AFAICT, this should be "convert PRintn int" and "convert PRUintn unsigned int". Further, if we want to be really paranoid, we should add a static_assert that (unsigned) int has at least the expected range of 32-bit (un)signed integers. > convert PROffset32 int32_t I suggest that we do not do the conversion of PROffset32 to int32_t. PROffset32 represents a 32-bit file offset, and it makes it very obvious that the code is not correct if given large files. converting usages of this type to int32_t makes this potential footgun much harder to recognize. > convert PROffset64 int64_t The C++0x type for PROffset64 is std::streamoff. Another common name for this type is off_t. I understand that one potential problem with these types is that they are not required to be 64-bits. IMO, that can probably be solved simply by a static_assert that streamoff or off_t has a signed 64-bit range. But, regardless, I think it would be nice to keep some indication of the intended usage of the type as a file offset in the name of the replacement type.

(In reply to comment #26) > > convert PRIntn int32_t > > convert PRUintn uint32_t > > AFAICT, this should be "convert PRintn int" and "convert PRUintn unsigned int". OK, I'll make this change. > Further, if we want to be really paranoid, we should add a static_assert that > (unsigned) int has at least the expected range of 32-bit (un)signed integers. That's outside of the scope of this bug, I think. I would be heavily surprised if we're not terribly broken on those hypothetical platforms already! ;-) > > convert PROffset32 int32_t > > I suggest that we do not do the conversion of PROffset32 to int32_t. PROffset32 > represents a 32-bit file offset, and it makes it very obvious that the code is > not correct if given large files. converting usages of this type to int32_t > makes this potential footgun much harder to recognize. PROffset32 is only used in two places outside of NSPR and NSS: bsdiff.c where it's used as file offsets among other things, and nsISupportsImpl.h where it is not used as a file offset. It's very easy to file a bug for the first case (although I'm not sure exactly what I would put in that bug, since we don't use bsdiff for large files), so I will still go ahead and convert PROffset32. > > convert PROffset64 int64_t > > The C++0x type for PROffset64 is std::streamoff. Another common name for this > type is off_t. I understand that one potential problem with these types is that > they are not required to be 64-bits. IMO, that can probably be solved simply by > a static_assert that streamoff or off_t has a signed 64-bit range. But, > regardless, I think it would be nice to keep some indication of the intended > usage of the type as a file offset in the name of the replacement type. I believe streamoff is only supposed to be used for <iostream> APIs, and StandardInteger.h doesn't support off_t. And to put things in perspective, PROffset64 is only used three times, all of them being used for the return value of PR_Seek64, so I think I will stick to int64_t for now.

Given that you're leaving PRUptrdiff alone already, I'd just leave the PROffsets alone too; those five places don't really bother use, do they?

(In reply to comment #28) > Given that you're leaving PRUptrdiff alone already, I'd just leave the > PROffsets alone too; those five places don't really bother use, do they? The reason that I am leaving PRUptrdiff out is that there is no stdint type corresponding to an unsigned offset (whatever that means!), it's not a matter of it bothering me! :-) I'll wait to see what bsmedberg thinks, I can easily leave them out if needed.

(In reply to Ehsan Akhgari [:ehsan] from comment #27) > (In reply to comment #26) > > > convert PRIntn int32_t > > > convert PRUintn uint32_t > > > > AFAICT, this should be "convert PRintn int" and "convert PRUintn unsigned int". > > OK, I'll make this change. Please don't. See the dev-platform thread as to why and discuss there if there is to discuss.

(In reply to Ehsan Akhgari [:ehsan] from comment #27) > PROffset32 is only used in two places outside of NSPR and NSS: bsdiff.c > where it's used as file offsets among other things, and nsISupportsImpl.h > where it is not used as a file offset. It's very easy to file a bug for the > first case (although I'm not sure exactly what I would put in that bug, > since we don't use bsdiff for large files), so I will still go ahead and > convert PROffset32. Bug 399549 is about upgrading to bsdiff 4.3 which uses off_t in its upstream source code, so no need for a new bug. > I believe streamoff is only supposed to be used for <iostream> APIs, and > StandardInteger.h doesn't support off_t. And to put things in perspective, > PROffset64 is only used three times, all of them being used for the return > value of PR_Seek64, so I think I will stick to int64_t for now. We use off_t in multiple places and we also support ctypes.off_t (bug 757462), so we should make sure off_t works in C++ in a reasonable way and use it, by making sure that off_t is always defined as a 64-bit int in all of our builds. (The patch for ctypes.off_t indicates that is not the case, as it supports 32-bit off_t, which IMO is a bug on its own.) Still, it's no reason to hold up this bug if others don't agree.

(In reply to comment #30) > (In reply to Ehsan Akhgari [:ehsan] from comment #27) > > (In reply to comment #26) > > > > convert PRIntn int32_t > > > > convert PRUintn uint32_t > > > > > > AFAICT, this should be "convert PRintn int" and "convert PRUintn unsigned int". > > > > OK, I'll make this change. > > Please don't. See the dev-platform thread as to why and discuss there if there > is to discuss. I didn't really understand the arguments being made there. Can you please explain it again?

Sorry, I was confused, I thought we were talking convert PRInt32 int. Forget what I said.

Comment on attachment 650886 [details] Part 6: c-c conversion script Stealing review...

Comment on attachment 650919 [details] [diff] [review] Part 7: Required includes for comm-central Stealing review...

Ehsan: The code is fine, but I have two questions: 1) Did you ever get a resolution on how to deal with PRUPtrdiff? 2) Part 4 manually converts to stdint types where the script didn't do the job. Do you think we're going to run into the same issue in comm-central? How did you detect these - just by landing all of these patches and seeing what didn't build? If so, we might want to do the same for comm-central. Let me know your position on these two questions, and I'll complete my r? accordingly. Thanks for looking out for c-c, -Mike

(In reply to comment #36) > Ehsan: > > The code is fine, but I have two questions: > > 1) Did you ever get a resolution on how to deal with PRUPtrdiff? No, and I'm ignoring that for now. > 2) Part 4 manually converts to stdint types where the script didn't do the job. > Do you think we're going to run into the same issue in comm-central? How did > you detect these - just by landing all of these patches and seeing what didn't > build? I detected those occurrences by just grepping the sources. The script only modifies C/C++/Objective-C files automatically, and ignores all the rest. Whether or not we want to detect the possible remaining usages is a matter of cleanliness, but I'm not planning to do that in this bug. > If so, we might want to do the same for comm-central. I will be happy to help someone else do that! :-)

(In reply to Ehsan Akhgari [:ehsan] from comment #37) > I detected those occurrences by just grepping the sources. The script only > modifies C/C++/Objective-C files automatically, and ignores all the rest. > Whether or not we want to detect the possible remaining usages is a matter > of cleanliness, but I'm not planning to do that in this bug. > > > If so, we might want to do the same for comm-central. > > I will be happy to help someone else do that! :-) Ok, I'm running your conversation script locally, and I'll grep around to see if I get any hits.

Comment on attachment 650886 [details] Part 6: c-c conversion script Does the job.

Comment on attachment 650919 [details] [diff] [review] Part 7: Required includes for comm-central Makes sense from inspection - I haven't tried building any of this, though.

Modified the script to convert PRIntn and PRUintn to int and unsigned, respectively.

The second version of part 5 seems to give us a green run on the try server.

mozilla-central changes: https://hg.mozilla.org/mozilla-central/rev/a16372ce30b5 https://hg.mozilla.org/mozilla-central/rev/c0726f9e6dc2 https://hg.mozilla.org/mozilla-central/rev/8e62cbb35f67 https://hg.mozilla.org/mozilla-central/rev/325891716910 https://hg.mozilla.org/mozilla-central/rev/88e47f6905e9 mozilla-inbound changes: https://hg.mozilla.org/integration/mozilla-inbound/rev/a16372ce30b5 https://hg.mozilla.org/integration/mozilla-inbound/rev/c0726f9e6dc2 https://hg.mozilla.org/integration/mozilla-inbound/rev/8e62cbb35f67 https://hg.mozilla.org/integration/mozilla-inbound/rev/325891716910 https://hg.mozilla.org/integration/mozilla-inbound/rev/88e47f6905e9 https://hg.mozilla.org/integration/mozilla-inbound/rev/9c39400dd21d https://hg.mozilla.org/integration/mozilla-inbound/rev/cd82204dcb67 comm-central changes: https://hg.mozilla.org/comm-central/rev/3f9b812e7247 https://hg.mozilla.org/comm-central/rev/5aa77ab8e201 https://hg.mozilla.org/comm-central/rev/0659a435dfad

Removed some of the NSPR types that crept in since yesterday: https://hg.mozilla.org/integration/mozilla-inbound/rev/ef0db3592a2e

Removed some more NSPR types that crept in... https://hg.mozilla.org/integration/mozilla-inbound/rev/d6aeeb24e99e

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #51) > https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c8020ccf94 Thanks for doing this. :-)

https://hg.mozilla.org/mozilla-central/rev/f0c8020ccf94 https://hg.mozilla.org/mozilla-central/rev/d6aeeb24e99e

Everyone is continuing to happily use NSPR types... :( https://hg.mozilla.org/integration/mozilla-inbound/rev/130e416aefd2

Is there any way to prevent further creeping in? (e.g. defining poison pills in mfbt)

(In reply to comment #56) > Is there any way to prevent further creeping in? > (e.g. defining poison pills in mfbt) We could do things like that, but they won't be effective for all cases.

(In reply to Ehsan Akhgari [:ehsan] from comment #57) > (In reply to comment #56) > > Is there any way to prevent further creeping in? > > (e.g. defining poison pills in mfbt) > > We could do things like that, but they won't be effective for all cases. So this patch starts to remove the include prtypes.h from nscore.h. It isn't a complete patch, but gives the basic idea. I think given that nscore.h (and hence prtypes.h) gets included virtually everywhere, it'd give a much better chance of developers hitting the case where PRInt32 etc isn't defined, and then getting used to using uint32_t etc. If folks then have to start including prtypes.h, that's an additional red flag to reviewers. Yes, we'd still need to include it in various places, and I haven't done any analysis on the amount of places that it would be not included with a complete patch, but I think it'd be a start to helping reducing the familiarity with the old types.

(In reply to Mark Banner (:standard8) from comment #59) > Created attachment 656791 [details] [diff] [review] > Possible way of reducing recurrances > > (In reply to Ehsan Akhgari [:ehsan] from comment #57) > > (In reply to comment #56) > > > Is there any way to prevent further creeping in? > > > (e.g. defining poison pills in mfbt) > > > > We could do things like that, but they won't be effective for all cases. > > So this patch starts to remove the include prtypes.h from nscore.h. It isn't > a complete patch, but gives the basic idea. > > I think given that nscore.h (and hence prtypes.h) gets included virtually > everywhere, it'd give a much better chance of developers hitting the case > where PRInt32 etc isn't defined, and then getting used to using uint32_t > etc. If folks then have to start including prtypes.h, that's an additional > red flag to reviewers. This is a great idea! I was approaching the problem the other way around (trying to reduce the stuff from prtypes.h that we use around the code base and then try to remove the prtypes.h includes), but this makes much more sense! > Yes, we'd still need to include it in various places, and I haven't done any > analysis on the amount of places that it would be not included with a > complete patch, but I think it'd be a start to helping reducing the > familiarity with the old types. Yeah, I agree. Can you please make this patch compile, and file another bug and attach it there? I would be happy to review it.

(In reply to Ehsan Akhgari [:ehsan] from comment #60) > Yeah, I agree. Can you please make this patch compile, and file another bug > and attach it there? I would be happy to review it. Filed bug 788014, but I won't have time to work on it for the next few weeks.

Another set of NSPR type removals: https://hg.mozilla.org/integration/mozilla-inbound/rev/42777635165a

Backed that out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1424662e3136 to back out one of the things it was cleaning up after.

Remove a couple PRBools from dom/ and xpcom/. https://tbpl.mozilla.org/?tree=Try&rev=982ac5145786

Comment on attachment 733194 [details] [diff] [review] remove-some-PRBools.patch Review of attachment 733194 [details] [diff] [review]: ----------------------------------------------------------------- Technically this is not the PRBool bug, but whatever. ;-) Thanks!

The battle goes on... https://hg.mozilla.org/integration/mozilla-inbound/rev/ef8e451f9c5a

Nit on the cset in comment 96: >- PRIntn openFlags; >+ int openFlags; [...skipping a bunch of lines...] > rv = path->OpenNSPRFileDesc(openFlags, 0644, &mFileDesc); It looks like the type of that OpenNSPRFileDesc parameter is technically int32_t (not int), based on the function signatures at the top of this MXR search: http://mxr.mozilla.org/mozilla-central/ident?i=OpenNSPRFileDesc

(In reply to Daniel Holbert [:dholbert] from comment #97) > Nit on the cset in comment 96: > >- PRIntn openFlags; > >+ int openFlags; > [...skipping a bunch of lines...] > > rv = path->OpenNSPRFileDesc(openFlags, 0644, &mFileDesc); > > It looks like the type of that OpenNSPRFileDesc parameter is technically > int32_t (not int), based on the function signatures at the top of this MXR > search: > http://mxr.mozilla.org/mozilla-central/ident?i=OpenNSPRFileDesc Yes. Blame whoever wrote the code using PRIntn. :-) Fixed in https://hg.mozilla.org/integration/mozilla-inbound/rev/0beea724bb11

Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/8155c0ef40e6 Part 1: Automated conversion of NSPR numeric types to stdint types in Gecko; r=bsmedberg