Last Comment Bug 579517 - (stdint) Use stdint types in gecko, replacing usage of NSPR types
(stdint)
: Use stdint types in gecko, replacing usage of NSPR types
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
: -- enhancement with 1 vote (vote)
: mozilla17
Assigned To: :Ehsan Akhgari (busy, don't ask for review please)
:
Mentors:
: 781229 785867 (view as bug list)
Depends on: 465641 704313 785066 785229 785738 788012 815359
Blocks: 550610 788014 792688 794186 828003
  Show dependency treegraph
 
Reported: 2010-07-16 13:36 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2016-01-01 16:37 PST (History)
27 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Script to unbitrot patches in mq (1.15 KB, text/plain)
2012-08-09 08:29 PDT, :Ehsan Akhgari (busy, don't ask for review please)
no flags Details
Part 1: Script to convert NSPR types to stdint types (1.02 KB, text/plain)
2012-08-09 08:31 PDT, :Ehsan Akhgari (busy, don't ask for review please)
no flags Details
Part 2: Make the IDL parser aware of stdint types (4.62 KB, patch)
2012-08-09 08:34 PDT, :Ehsan Akhgari (busy, don't ask for review please)
benjamin: review+
Details | Diff | Review
Part 3: Remove NSPR types from the IPDL parser's built-in type list (731 bytes, patch)
2012-08-09 08:35 PDT, :Ehsan Akhgari (busy, don't ask for review please)
benjamin: review+
Details | Diff | Review
Part 4: Manually rewrite some parts of the code base not covered by the automated conversion (125.81 KB, patch)
2012-08-09 08:38 PDT, :Ehsan Akhgari (busy, don't ask for review please)
benjamin: review+
Details | Diff | Review
Part 5: Add missing StandardInteger.h #includes (14.70 KB, patch)
2012-08-09 08:48 PDT, :Ehsan Akhgari (busy, don't ask for review please)
no flags Details | Diff | Review
Part 6: c-c conversion script (1.00 KB, text/plain)
2012-08-10 08:10 PDT, :Ehsan Akhgari (busy, don't ask for review please)
mconley: review+
Details
Part 7: Required includes for comm-central (937 bytes, patch)
2012-08-10 09:58 PDT, :Ehsan Akhgari (busy, don't ask for review please)
mconley: review+
Details | Diff | Review
Part 1: Script to convert NSPR types to stdint types (1.01 KB, text/plain)
2012-08-21 14:23 PDT, :Ehsan Akhgari (busy, don't ask for review please)
benjamin: review+
Details
Part 5: Add missing StandardInteger.h #includes (15.18 KB, patch)
2012-08-21 21:54 PDT, :Ehsan Akhgari (busy, don't ask for review please)
benjamin: review+
Details | Diff | Review
Possible way of reducing recurrances (14.88 KB, patch)
2012-08-30 02:58 PDT, Mark Banner (:standard8)
no flags Details | Diff | Review
remove-some-PRBools.patch (2.66 KB, patch)
2013-04-03 22:51 PDT, Chris Peterson [:cpeterson]
ehsan: review+
Details | Diff | Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-07-16 13:36:56 PDT
See bug 550610 for rationale.  It seems prudent to nick the build sorcery required for this from jsstdint.h.
Comment 1 Ted Mielczarek [:ted.mielczarek] 2010-07-21 10:15:19 PDT
To be clear, this would be "Replace NSPR types with stdint types"?
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-07-21 10:38:37 PDT
Yes, "Replace not-stdint types with stdint types".
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2011-09-10 17:07:02 PDT
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.
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-11 18:56:17 PDT
(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.
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2011-09-12 00:36:39 PDT
(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.
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2011-09-12 00:37:49 PDT
Here I was having only 2) in mind. You're entirely right about 1).
Comment 7 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-11-18 15:33:08 PST
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).
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-18 15:55:48 PST
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.
Comment 9 Luke Wagner [:luke] 2011-11-18 16:16:31 PST
(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.
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-18 16:57:33 PST
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
Comment 11 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-11-27 21:39:30 PST
(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.
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-28 23:48:00 PST
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.
Comment 13 Landry Breuil (:gaston) 2012-05-22 07:38:12 PDT
(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.
Comment 14 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-22 09:38:50 PDT
(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
Comment 15 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-08 09:58:32 PDT
*** Bug 781229 has been marked as a duplicate of this bug. ***
Comment 16 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-09 08:29:36 PDT
Created attachment 650572 [details]
Script to unbitrot patches in mq
Comment 17 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-09 08:31:38 PDT
Created attachment 650574 [details]
Part 1: Script to convert NSPR types to stdint types

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.
Comment 18 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-09 08:34:29 PDT
Created attachment 650575 [details] [diff] [review]
Part 2: Make the IDL parser aware of stdint types

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.
Comment 19 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-09 08:35:48 PDT
Created attachment 650576 [details] [diff] [review]
Part 3: Remove NSPR types from the IPDL parser's built-in type list

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.
Comment 20 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-09 08:38:58 PDT
Created attachment 650580 [details] [diff] [review]
Part 4: Manually rewrite some parts of the code base not covered by the automated conversion

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.
Comment 21 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-09 08:48:54 PDT
Created attachment 650586 [details] [diff] [review]
Part 5: Add missing StandardInteger.h #includes

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 22 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-09 11:23:35 PDT
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 23 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-09 12:09:03 PDT
Comment on attachment 650575 [details] [diff] [review]
Part 2: Make the IDL parser aware of stdint types

Actually this is still good!
Comment 24 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-10 08:10:37 PDT
Created attachment 650886 [details]
Part 6: c-c conversion script

This script moves comm-central over to stdint types as well.  Let there be a race on who reviews this sooner!
Comment 25 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-10 09:58:39 PDT
Created attachment 650919 [details] [diff] [review]
Part 7: Required includes for comm-central
Comment 26 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-08-10 10:40:14 PDT
> 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.
Comment 27 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-10 10:59:24 PDT
(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.
Comment 28 :Ms2ger 2012-08-10 11:58:40 PDT
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?
Comment 29 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-10 12:12:46 PDT
(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.
Comment 30 Mike Hommey [:glandium] 2012-08-10 12:32:58 PDT
(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.
Comment 31 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-08-10 12:49:37 PDT
(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.
Comment 32 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-10 13:55:32 PDT
(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?
Comment 33 Mike Hommey [:glandium] 2012-08-10 23:41:14 PDT
Sorry, I was confused, I thought we were talking convert PRInt32 int. Forget what I said.
Comment 34 Mike Conley (:mconley) - (needinfo me!) 2012-08-14 08:18:33 PDT
Comment on attachment 650886 [details]
Part 6: c-c conversion script

Stealing review...
Comment 35 Mike Conley (:mconley) - (needinfo me!) 2012-08-14 08:18:57 PDT
Comment on attachment 650919 [details] [diff] [review]
Part 7: Required includes for comm-central

Stealing review...
Comment 36 Mike Conley (:mconley) - (needinfo me!) 2012-08-14 08:27:40 PDT
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
Comment 37 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-14 08:34:30 PDT
(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!  :-)
Comment 38 Mike Conley (:mconley) - (needinfo me!) 2012-08-14 08:39:55 PDT
(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 39 Mike Conley (:mconley) - (needinfo me!) 2012-08-14 08:52:53 PDT
Comment on attachment 650886 [details]
Part 6: c-c conversion script

Does the job.
Comment 40 Mike Conley (:mconley) - (needinfo me!) 2012-08-14 08:53:25 PDT
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.
Comment 41 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-21 14:23:07 PDT
Created attachment 653946 [details]
Part 1: Script to convert NSPR types to stdint types

Modified the script to convert PRIntn and PRUintn to int and unsigned, respectively.
Comment 42 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-21 15:52:06 PDT
https://tbpl.mozilla.org/?tree=Try&rev=082d81908b4c
Comment 43 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-21 21:54:36 PDT
Created attachment 654090 [details] [diff] [review]
Part 5: Add missing StandardInteger.h #includes
Comment 44 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-21 21:54:54 PDT
https://tbpl.mozilla.org/?tree=Try&rev=e15d5d68e303
Comment 45 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-22 07:22:18 PDT
The second version of part 5 seems to give us a green run on the try server.
Comment 47 Ed Morley [:emorley] 2012-08-23 03:50:39 PDT
https://hg.mozilla.org/mozilla-central/rev/cd82204dcb67
Comment 48 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-23 08:32:08 PDT
Removed some of the NSPR types that crept in since yesterday:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef0db3592a2e
Comment 49 Ryan VanderMeulen [:RyanVM] 2012-08-23 19:20:52 PDT
https://hg.mozilla.org/mozilla-central/rev/ef0db3592a2e
Comment 50 Vicamo Yang [:vicamo][:vyang] 2012-08-27 07:26:12 PDT
*** Bug 785867 has been marked as a duplicate of this bug. ***
Comment 51 Vicamo Yang [:vicamo][:vyang] 2012-08-27 08:32:38 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c8020ccf94
Comment 52 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-27 09:32:17 PDT
Removed some more NSPR types that crept in... https://hg.mozilla.org/integration/mozilla-inbound/rev/d6aeeb24e99e
Comment 53 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-27 09:35:17 PDT
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #51)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c8020ccf94

Thanks for doing this.  :-)
Comment 55 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-29 08:26:55 PDT
Everyone is continuing to happily use NSPR types... :(
https://hg.mozilla.org/integration/mozilla-inbound/rev/130e416aefd2
Comment 56 Masatoshi Kimura [:emk] 2012-08-29 08:39:27 PDT
Is there any way to prevent further creeping in?
(e.g. defining poison pills in mfbt)
Comment 57 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-29 10:10:29 PDT
(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.
Comment 58 Ryan VanderMeulen [:RyanVM] 2012-08-29 17:19:03 PDT
https://hg.mozilla.org/mozilla-central/rev/130e416aefd2
Comment 59 Mark Banner (:standard8) 2012-08-30 02:58:35 PDT
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.

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.
Comment 60 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-30 15:35:46 PDT
(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.
Comment 61 Mark Banner (:standard8) 2012-09-03 14:04:45 PDT
(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.
Comment 62 :Ehsan Akhgari (busy, don't ask for review please) 2012-09-06 07:13:24 PDT
Another set of NSPR type removals:

https://hg.mozilla.org/integration/mozilla-inbound/rev/42777635165a
Comment 63 Ryan VanderMeulen [:RyanVM] 2012-09-06 18:12:08 PDT
https://hg.mozilla.org/mozilla-central/rev/42777635165a
Comment 64 :Ehsan Akhgari (busy, don't ask for review please) 2012-09-07 16:10:12 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0d849392a8e
Comment 65 Ryan VanderMeulen [:RyanVM] 2012-09-07 21:15:05 PDT
https://hg.mozilla.org/mozilla-central/rev/a0d849392a8e
Comment 66 :Ehsan Akhgari (busy, don't ask for review please) 2012-09-10 11:12:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c8189bcb390
Comment 67 Ryan VanderMeulen [:RyanVM] 2012-09-10 18:45:14 PDT
https://hg.mozilla.org/mozilla-central/rev/9c8189bcb390
Comment 68 :Ehsan Akhgari (busy, don't ask for review please) 2012-09-14 11:56:06 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b3f5ceb618a
Comment 69 Ryan VanderMeulen [:RyanVM] 2012-09-14 18:18:00 PDT
https://hg.mozilla.org/mozilla-central/rev/0b3f5ceb618a
Comment 70 :Ehsan Akhgari (busy, don't ask for review please) 2012-09-24 14:55:38 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bd13443d0bc
Comment 71 Phil Ringnalda (:philor) 2012-09-24 21:31:41 PDT
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.
Comment 72 :Ehsan Akhgari (busy, don't ask for review please) 2012-10-01 13:04:58 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f134e05d0336
Comment 73 Ryan VanderMeulen [:RyanVM] 2012-10-01 18:58:47 PDT
https://hg.mozilla.org/mozilla-central/rev/f134e05d0336
Comment 74 :Ehsan Akhgari (busy, don't ask for review please) 2012-10-07 15:26:43 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/9827af2a97ce
Comment 75 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-08 01:33:42 PDT
https://hg.mozilla.org/mozilla-central/rev/9827af2a97ce
Comment 76 :Ehsan Akhgari (busy, don't ask for review please) 2012-10-25 11:45:26 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/05e44f47d1bf
Comment 77 Ryan VanderMeulen [:RyanVM] 2012-10-25 18:28:17 PDT
https://hg.mozilla.org/mozilla-central/rev/05e44f47d1bf
Comment 78 :Ehsan Akhgari (busy, don't ask for review please) 2012-11-07 17:35:54 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/7330be7e07a3
Comment 79 Ed Morley [:emorley] 2012-11-08 02:40:30 PST
https://hg.mozilla.org/mozilla-central/rev/7330be7e07a3
Comment 80 :Ehsan Akhgari (busy, don't ask for review please) 2013-01-07 15:22:45 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/63d127cca94c
Comment 81 Ed Morley [:emorley] 2013-01-08 04:30:37 PST
https://hg.mozilla.org/mozilla-central/rev/63d127cca94c
Comment 82 :Ehsan Akhgari (busy, don't ask for review please) 2013-04-02 18:00:33 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb7aae8421bc
Comment 83 Ed Morley [:emorley] 2013-04-03 03:42:16 PDT
https://hg.mozilla.org/mozilla-central/rev/fb7aae8421bc
Comment 84 Chris Peterson [:cpeterson] 2013-04-03 22:51:08 PDT
Created attachment 733194 [details] [diff] [review]
remove-some-PRBools.patch

Remove a couple PRBools from dom/ and xpcom/.

https://tbpl.mozilla.org/?tree=Try&rev=982ac5145786
Comment 85 :Ehsan Akhgari (busy, don't ask for review please) 2013-04-04 09:36:18 PDT
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!
Comment 86 Chris Peterson [:cpeterson] 2013-04-04 10:30:04 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc3f63dbba94
Comment 87 Ryan VanderMeulen [:RyanVM] 2013-04-04 18:16:40 PDT
https://hg.mozilla.org/mozilla-central/rev/bc3f63dbba94
Comment 88 :Ehsan Akhgari (busy, don't ask for review please) 2013-06-12 18:28:39 PDT
The battle goes on...

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef8e451f9c5a
Comment 89 Ed Morley [:emorley] 2013-06-13 02:30:49 PDT
https://hg.mozilla.org/mozilla-central/rev/ef8e451f9c5a
Comment 90 :Ehsan Akhgari (busy, don't ask for review please) 2013-06-23 18:01:02 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/76820c6dff7b
Comment 91 Phil Ringnalda (:philor) 2013-06-23 22:55:36 PDT
https://hg.mozilla.org/mozilla-central/rev/76820c6dff7b
Comment 92 :Ehsan Akhgari (busy, don't ask for review please) 2013-08-02 09:10:56 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/3719e3ea26b2
Comment 93 Ryan VanderMeulen [:RyanVM] 2013-08-02 14:21:11 PDT
https://hg.mozilla.org/mozilla-central/rev/3719e3ea26b2
Comment 94 :Ehsan Akhgari (busy, don't ask for review please) 2013-09-22 16:26:59 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d72b3d939bbc
Comment 95 Carsten Book [:Tomcat] 2013-09-23 04:16:34 PDT
https://hg.mozilla.org/mozilla-central/rev/d72b3d939bbc
Comment 96 :Ehsan Akhgari (busy, don't ask for review please) 2014-01-03 16:25:17 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/42ac1de2e3f7
Comment 97 Daniel Holbert [:dholbert] 2014-01-03 17:59:43 PST
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
Comment 98 :Ehsan Akhgari (busy, don't ask for review please) 2014-01-06 07:08:29 PST
(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
Comment 99 Ryan VanderMeulen [:RyanVM] 2014-01-06 11:06:27 PST
https://hg.mozilla.org/mozilla-central/rev/42ac1de2e3f7
Comment 100 :Ehsan Akhgari (busy, don't ask for review please) 2014-08-08 05:42:00 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0c00c1861af
Comment 101 Wes Kocher (:KWierso) 2014-08-08 14:37:35 PDT
https://hg.mozilla.org/mozilla-central/rev/e0c00c1861af
Comment 102 Carsten Book [:Tomcat] 2015-03-17 03:48:48 PDT
https://hg.mozilla.org/mozilla-central/rev/579c3e8855b4
Comment 104 Ryan VanderMeulen [:RyanVM] 2016-01-01 16:37:15 PST
https://hg.mozilla.org/mozilla-central/rev/9d748eaec52f

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