802430 - Allow NSS to be built with NO_NSPR_10_SUPPORT

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

See bug 794510 where this was done for mozilla-central.

It seems like there may be many simple changes to replace uses of PRArenaPool with PLArenaPool.

Isaac, if you are interested in doing these changes, let me know if you run into any problems. Here are some things to keep in mind, that are different than the rest of mozilla-central:

1. It is better to do the changes on the NSS trunk:

   export CVSROOT=:pserver:[e-mail]:/cvsroot
   cvs co NSPR NSS
   cd mozilla/security/nss
   make nss_build_all
   cd tests
   HOST=localhost ./all.sh to run the tests

2. NSS uses tabs for indention. Avoid changing the tabs to spaces.

3. We'll update the copy of NSS in mozilla-central separately.

4. You can post your patch in Mercurial/Git format or "diff -u8p" format.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: [CHECKED IN] Replace PRArenaPool with PLArenaPool in nss/cmd/lib/secutil.h

This is needed for Mozilla bug 794510. I am not sure why Mozilla's Android code is including code from nss/cmd/lib/secutil.h in the first place. Is this actually considered a public header?

Comment 2

[Isaac Aggrey [:isaac]]

(In reply to Brian Smith (:bsmith) from comment #0)
> Isaac, if you are interested in doing these changes, let me know if you run
> into any problems.

I'm too slow, it looks like you got it now. ;-)

Comment 3

[Wan-Teh Chang]

Comment on attachment 672168 [details] [diff] [review]
[CHECKED IN] Replace PRArenaPool with PLArenaPool in nss/cmd/lib/secutil.h

Review of attachment 672168 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. There may be a code freeze on the NSS trunk now.
We should find out whether NSS 3.14 is already finished
or we are planning to make another release candidate (RC1).

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Isaac Aggrey [:isaac] from comment #2)
> (In reply to Brian Smith (:bsmith) from comment #0)
> > Isaac, if you are interested in doing these changes, let me know if you run
> > into any problems.
> 
> I'm too slow, it looks like you got it now. ;-)

Isaac, I only changed one header. There are many other changes that would be necessary to resolve this bug. But, I am not sure this is something that is as straightforward as we thought.

The main complication is that NSS exposes a function in its stable API, CERT_GenTime2FormattedAscii, that is just a very thin wrapper around PR_FormatTime. So, basically, NSS requires PR_FormatTime forever. Accordingly, I think that we should change NSPR to allow programs that use PR_FormatTime to be built with NO_NSPR_10_SUPPORT.

A more minor complication is that we may want to avoid changing dbm to build with NO_NSPR_10_SUPPORT, because dbm uses the old types (uint, uint8, uint16, uint32, uint64, etc.) frequently, and changing that code to use NSPR types requires reformatting the code and/or just accepting that the pretty alignment of declarations in the dbm code will be ruined.

Note that just changing PRArenaPool -> PLArenaPool touches many lines of code:

in security/nss/lib/**/*.h:  88 lines in  28 different files.
in security/nss/lib/**/*.c: 549 lines in 110 different files.
in security/nss/cmd/**/*.*: 123 lines in  23 different files.

Comment 5

[Kai Engert (:KaiE:)]

Checking in secutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v  <--  secutil.h
new revision: 1.49; previous revision: 1.48
done

Comment 6

[(no longer active)]

(In reply to Brian Smith (:bsmith) from comment #4)
> (In reply to Isaac Aggrey [:isaac] from comment #2)
> > (In reply to Brian Smith (:bsmith) from comment #0)
> > > Isaac, if you are interested in doing these changes, let me know if you run
> > > into any problems.
> > 
> > I'm too slow, it looks like you got it now. ;-)
> 
> Isaac, I only changed one header. There are many other changes that would be
> necessary to resolve this bug. But, I am not sure this is something that is
> as straightforward as we thought.
> 
> The main complication is that NSS exposes a function in its stable API,
> CERT_GenTime2FormattedAscii, that is just a very thin wrapper around
> PR_FormatTime. So, basically, NSS requires PR_FormatTime forever.
> Accordingly, I think that we should change NSPR to allow programs that use
> PR_FormatTime to be built with NO_NSPR_10_SUPPORT.

Hmm, PR_FormatTime itself is a thin wrapper around libc's strftime <http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/misc/prtime.c#1675>.  Can't we just use that directly?

If not, what you suggest here makes sense to me too.

> A more minor complication is that we may want to avoid changing dbm to build
> with NO_NSPR_10_SUPPORT, because dbm uses the old types (uint, uint8,
> uint16, uint32, uint64, etc.) frequently, and changing that code to use NSPR
> types requires reformatting the code and/or just accepting that the pretty
> alignment of declarations in the dbm code will be ruined.

FWIW, that can be achieved with sed rules like this:

s/\buint8  /PRUint8/
s/\buint8\b/PRUint8/

etc.

> Note that just changing PRArenaPool -> PLArenaPool touches many lines of
> code:
> 
> in security/nss/lib/**/*.h:  88 lines in  28 different files.
> in security/nss/lib/**/*.c: 549 lines in 110 different files.
> in security/nss/cmd/**/*.*: 123 lines in  23 different files.

Yeah, it code churn is a concern, this will be a non-starter.  :/

Comment 7

[Wan-Teh Chang]

Attachment: Patch for dbm

I think the dbm code is not worth fixing. If necessary, we can
add the typedefs to mozilla/dbm/include/mcom_db.h. Here is a
patch to do that.

I am willing to allow PR_FormatTime and PR_FormatTimeUSEnglish
in NO_NSPR_10_SUPPORT code.

Comment 8

[Wan-Teh Chang]

Attachment: Part 0: Patch for dbm, v2

Windows needs the uint type, which is defined in <sys/types.h>
on Linux.

I forgot to explain the removal of $(SECURITY_FLAG). As this
MXR query shows, SECURITY_FLAG is never defined:
http://mxr.mozilla.org/security/search?string=SECURITY_FLAG

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 673491 [details] [diff] [review]
Part 0: Patch for dbm, v2

Review of attachment 673491 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: mozilla/security/dbm/src/config.mk
@@ +4,4 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +DEFINES += -DMEMMOVE -D__DBINTERFACE_PRIVATE -DNO_NSPR_10_SUPPORT

I noticed that dbm also picks up the DEFINES from coreconf/config.mk, so when all of NSS is changed to support -DNO_NSPR_10_SUPPORT, this might become redundant.

Comment 10

[Isaac Aggrey [:isaac]]

(In reply to Brian Smith (:bsmith) from comment #4)
> Isaac, I only changed one header. There are many other changes that would be
> necessary to resolve this bug. But, I am not sure this is something that is
> as straightforward as we thought.

My mistake, all I saw was a patch already there and made a quick judgment without looking. :-)

In that case, I'm willing to take this on since it's along the lines of bug 794510.

Comment 11

[Isaac Aggrey [:isaac]]

Attachment: Part 1: Build NSS with NO_NSPR_10_SUPPORT

Comment 12

[Isaac Aggrey [:isaac]]

Attachment: Part 2: Use plarena.h types

Comment 13

[Isaac Aggrey [:isaac]]

Attachment: Part 3: Use prtypes.h int types

Comment 14

[Isaac Aggrey [:isaac]]

Attachment: [CHECKED IN] Part 4: Allow PR_FormatTime and PR_FormatTimeUSEnglish

Based on Wan-Teh's comment 7, the PR_FormatTime and PR_FormatTimeUSEnglish functions are now allowed -- unless we want to pursue what Ehsan suggested in comment 6.

Also, on removal of the FIXME comment, I may be mistaken but it seemed to refer to the PR_FormatTime functions, so I removed that as well.

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 674333 [details] [diff] [review]
[CHECKED IN] Part 4: Allow PR_FormatTime and PR_FormatTimeUSEnglish

LGTM, but Wan-Teh might want some additional notes regarding the localization issues, so please get Wan-Teh's review.

Comment 16

[Isaac Aggrey [:isaac]]

Attachment: Part 3: Use prtypes.h types, v2

A couple formatting issues fixed, but I also forgot to mention a couple things about the non-integer changes:

* PR_BITS_PER_BYTE was required instead of BITS_PER_BYTE, but I couldn't exactly figure out why from just including NO_NSPR_10_SUPPORT (FWIW, other NSS code uses PR_BITS_PER_BYTE)

* PR_IMPLEMENT replaced PR_PUBLIC_API, which is undefined under NO_NSPR_10_SUPPORT

Comment 17

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 674325 [details] [diff] [review]
Part 3: Use prtypes.h int types

Review of attachment 674325 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/nss/cmd/fipstest/fipstest.c
@@ +3310,5 @@
>      HASH_HashType hashType = HASH_AlgNULL;
>  
>      switch (hashbits) {
>      case 1:
> +    case (SHA1_LENGTH*PR_BITS_PER_BYTE): 

Look at the the patch in Splinter and make sure you remove the trailing whitespace from the lines you change (and only those lines).

::: security/nss/cmd/ocspclnt/ocspclnt.c
@@ +255,5 @@
>  {
>      CERTCertList *certs = NULL;
>      CERTCertificate *myCert = NULL;
>      CERTOCSPRequest *request = NULL;
> +    PRInt64 now = PR_Now();

PRTime now = PR_Now();

There are multiple instances of this, in this file and other files.

When an int64 is being used as a time, use PRTime. When in doubt, you can assume that in NSS, if int64 is being used, it is a PRTime.

::: security/nss/lib/freebl/blapi.h
@@ +1068,5 @@
>  			unsigned int inputLen);
>  extern void SHA224_End(SHA224Context *cx, unsigned char *digest,
>  		     unsigned int *digestLen, unsigned int maxDigestLen);
>  extern SECStatus SHA224_HashBuf(unsigned char *dest, const unsigned char *src,
> +			      PRUint32 src_length);

Please fix the indention of the lines you changed (only) too. (Remember to use tabs.)

This applies to the following declarations too.

::: security/nss/lib/freebl/loader.h
@@ +243,5 @@
>  			unsigned int inputLen);
>   void (* p_SHA256_End)(SHA256Context *cx, unsigned char *digest,
>  		     unsigned int *digestLen, unsigned int maxDigestLen);
>   SECStatus (* p_SHA256_HashBuf)(unsigned char *dest, const unsigned char *src,
> +			      PRUint32 src_length);

Please fix the indention of the lines you changed in this file too.

::: security/nss/lib/freebl/pqg.c
@@ +486,5 @@
>  **
>  ** This implments steps 4 thorough 22 of FIPS 186-3 A.1.2.1 and
>  **                steps 16 through 34 of FIPS 186-2 C.6
>  */
> +#define MAX_ST_SEED_BITS HASH_LENGTH_MAX*PR_BITS_PER_BYTE

Please also add the parentheses:
#define MAX_ST_SEED_BITS (HASH_LENGTH_MAX*PR_BITS_PER_BYTE)

@@ +1010,5 @@
>      CHECK_MPI_OK( mp_init(&tmp)  );
>      CHECK_MPI_OK( mp_init(&V_n)  );
>  
>      hashlen = HASH_ResultLen(hashtype);
> +    outlen = hashlen*PR_BITS_PER_BYTE; 

more trailing whitespace.

::: security/nss/lib/freebl/sha512.c
@@ +1337,5 @@
>  }
>  
>  SECStatus 
>  SHA384_HashBuf(unsigned char *dest, const unsigned char *src,
> +			  PRUint32 src_length)

Please fix the indention while you are here.

::: security/nss/lib/freebl/unix_rand.c
@@ +613,5 @@
>  static void
>  GiveSystemInfo(void)
>  {
>      system_info *info = NULL;
> +    PRInt32 val;                     

Please fix trailing whitespace.

::: security/nss/lib/jar/jzlib.h
@@ +173,5 @@
>  
>                          /* basic functions */
>  
>  #ifdef MOZILLA_CLIENT
> +PR_IMPLEMENT(extern const char *) zlibVersion (void);

PR_EXTERN should be used in headers and PR_IMPLEMENT in the *.c file. See the documentation for PR_EXTERN/PR_IMPLEMENT in prtypes.h.

::: security/nss/lib/nss/utilwrap.c
@@ +447,5 @@
>      return DER_TimeToGeneralizedTime_Util(dst, gmttime);
>  }
>  
>  SECStatus DER_TimeToGeneralizedTimeArena(PLArenaPool* arenaOpt,
> +                                                SECItem *dst, PRInt64 gmttime)

Fix the indention while you are here.

@@ +457,5 @@
>  {
>      return DER_GeneralizedTimeToTime_Util(dst, time);
>  }
>  
> +char *CERT_GenTime2FormattedAscii (PRInt64 genTime, char *format)

Remove space between "Ascii" and the parameters while you are here.

::: security/nss/lib/ssl/ssl3con.c
@@ +3514,5 @@
>  ssl3_AppendHandshakeNumber(sslSocket *ss, PRInt32 num, PRInt32 lenSize)
>  {
>      SECStatus rv;
> +    PRUint8   b[4];
> +    PRUint8 *   p = b;

Keep p aligned with b.

@@ +7610,5 @@
>  ssl3_SendCertificateRequest(sslSocket *ss)
>  {
>      SECItem *      name;
>      CERTDistNames *ca_list;
> +    const PRUint8 *  certTypes;

const PRUInt8 *certTypes;

::: security/nss/lib/ssl/ssl3ext.c
@@ +88,5 @@
>  ssl3_AppendNumberToItem(SECItem *item, PRUint32 num, PRInt32 lenSize)
>  {
>      SECStatus rv;
> +    PRUint8   b[4];
> +    PRUint8 *   p = b;

align p with b.

::: security/nss/lib/util/sectime.c
@@ +52,5 @@
>      }
>  }
>  
>  char *
> +CERT_UTCTime2FormattedAscii (PRInt64 utcTime, char *format)

Remove the space before the parameters.

@@ +72,5 @@
>      
>      return (timeString);
>  }
>  
> +char *CERT_GenTime2FormattedAscii (PRInt64 genTime, char *format)

Remove the space before the paraemters.

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 674353 [details] [diff] [review]
Part 3: Use prtypes.h types, v2

Please address the comments from v1 and submit a v3 for review

Comment 19

[Wan-Teh Chang]

Comment on attachment 673491 [details] [diff] [review]
Part 0: Patch for dbm, v2

Brian: yes, we will need to remove -DNO_NSPR_10_SUPPORT from
mozilla/security/dbm/src/config.mk when we add -DNO_NSPR_10_SUPPORT
to coreconf/config.mk.

Patch checked in on the NSS trunk (NSS 3.14.1).

Checking in mozilla/dbm/include/mcom_db.h;
/cvsroot/mozilla/dbm/include/mcom_db.h,v  <--  mcom_db.h
new revision: 3.45; previous revision: 3.44
done
Checking in mozilla/security/dbm/src/config.mk;
/cvsroot/mozilla/security/dbm/src/config.mk,v  <--  config.mk
new revision: 1.9; previous revision: 1.8
done

Comment 20

[Wan-Teh Chang]

Comment on attachment 674333 [details] [diff] [review]
[CHECKED IN] Part 4: Allow PR_FormatTime and PR_FormatTimeUSEnglish

r=wtc. I moved this patch to NSPR bug 804833 and checked it in.

Comment 21

[Isaac Aggrey [:isaac]]

Attachment: Part 2: Use plarena.h types, v2

Fixed trailing whitespace

Comment 22

[Isaac Aggrey [:isaac]]

Attachment: Part 3: Use prtypes.h types, v3

Fixed comment 17 issues

Comment 23

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 674321 [details] [diff] [review]
Part 1: Build NSS with NO_NSPR_10_SUPPORT

Review of attachment 674321 [details] [diff] [review]:
-----------------------------------------------------------------

Changes to NSS from new people require r+ and sr+. When you post the replacement, please mark it r+ and ask Wan-Teh for sr?.

::: security/coreconf/config.mk
@@ +174,5 @@
>  # in libnss3 which are present for binary compatibility only
>  DEFINES += -DUSE_UTIL_DIRECTLY
>  USE_UTIL_DIRECTLY = 1
>  
> +# Build with NO_NSPR_10_SUPPORT to avoid using obsolete protypes.h

s/protypes.h/NSPR features/

Comment 24

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Fix dbm build by replacing "uint" with "unsigned int"

I misunderstood your comment about Windows needing uint; I thought there would be a follow-up patch to add that. In any case, I found that POSIX doesn't guarantee uint to be in sys/types.h, so instead of #ifdef'ing around, I just changed all references of "uint" to "unsigned int" in dbm, on the assumption that wherever uint is defined, it is defined to be "unsigned int".

I chose to preserve the alignment of the member names and comments in structure declarations, instead of the alignemnt of the type names. Note that the patch looks terrible because of the mixing of tabs and spaces and how the "+", "-", or " " at the start of each patch line interacts with whitespace. But, I verified that the final results are lined up as prettily as I could make them.

Comment 25

[Wan-Teh Chang]

Comment on attachment 673491 [details] [diff] [review]
Part 0: Patch for dbm, v2

I backed out my checkin for two reasons. 1) I checked in the older
version of this patch by mistake. 2) This patch needs to wait until
mozilla/security/nss/lib/softoken/legacydb can be compiled with
NO_NSPR_10_SUPPORT because it also includes mcom_db.h.

Checking in mozilla/dbm/include/mcom_db.h;
/cvsroot/mozilla/dbm/include/mcom_db.h,v  <--  mcom_db.h
new revision: 3.46; previous revision: 3.45
done
Checking in mozilla/security/dbm/src/config.mk;
/cvsroot/mozilla/security/dbm/src/config.mk,v  <--  config.mk
new revision: 1.10; previous revision: 1.9
done

Comment 26

[Wan-Teh Chang]

Comment on attachment 674507 [details] [diff] [review]
Fix dbm build by replacing "uint" with "unsigned int"

Brian: sorry about my mistake. This patch isn't necessary.
I checked in the older version of my dbm patch, which didn't
deal with the uint typedef.

Comment 27

[Isaac Aggrey [:isaac]]

Attachment: Part 1: Build NSS with NO_NSPR_10_SUPPORT

Comment 28

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: [NOT FOR CHECKIN] Changes between the rolled-up patches and previously-posted patches

I took all of the previously-posted patches, applied then, made some minor changes, and created a rolled-up patch. This is the interdiff between the previously-posted patches and that rolled-up patch.

Comment 29

[Isaac Aggrey [:isaac]]

(In reply to Brian Smith (:bsmith) from comment #28)
> Created attachment 674514 [details] [diff] [review]
> [NOT FOR CHECKIN] Changes between the rolled-up patches and
> previously-posted patches
> 
> I took all of the previously-posted patches, applied then, made some minor
> changes, and created a rolled-up patch. This is the interdiff between the
> previously-posted patches and that rolled-up patch.

Sorry about that, Brian. From your patch, I see I didn't catch all of the formatting issues or use PR_EXTERN correctly in my patch. 

Since this patch is not for checkin, should I make those corrections?

Comment 30

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: All NSS patches rolled up into one

I found that "unsigned int" was more common in NSS than PRUintn so I changed Isaac's patch to use unsigned int.

I corrected "PR_EXTERN(extern " to "PR_EXTERN(" and fixed some alignment issues in that part of the patch. It seems a previous author wanted to have some related declarations aligned so I mostly aligned them.

I fixed the comment in coreconf/config.mk as I asked for in my review comment.

I removed the now-redundant -DNO_NSPR_10_SUPPORT in dbm's config.mk.

To review the mass changes, I did a case-sensitive, whole-word find/replace for the obvious changes in a clean tree and then diff'd against what Isaac did to verify they matched, manually reviewing (and occasionally adjusting) the whitespace changes.

Comment 31

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 674517 [details] [diff] [review]
All NSS patches rolled up into one

This patch is really Isaac's patch with me fixing my review comments myself, instead of forcing Isaac to do it.

(In reply to Wan-Teh Chang from comment #25)
> I backed out my checkin for two reasons. 1) I checked in the older
> version of this patch by mistake. 2) This patch needs to wait until
> mozilla/security/nss/lib/softoken/legacydb can be compiled with
> NO_NSPR_10_SUPPORT because it also includes mcom_db.h.

The patch to get all of NSS to build with NO_NSPR_10_SUPPORT also depends on the changes to mcom_db.h. With your "bad" dbm v2 patch + this patch, NSS will build with NO_NSPR_10_SUPPORT defined.

Isaac, it isn't necessary to post any more follow-ups yet, because I made all of the fixes in the roll-up patch.

Comment 32

[Wan-Teh Chang]

Comment on attachment 674517 [details] [diff] [review]
All NSS patches rolled up into one

Review of attachment 674517 [details] [diff] [review]:
-----------------------------------------------------------------

Brian: the only change necessary for mozilla/dbm and mozilla/security/dbm
is my mcom_db.h change in attachment 673491 [details] [diff] [review]. It is not necessary to change
'uint' to 'unsigned int'. Thanks.

Comment 33

[Wan-Teh Chang]

Comment on attachment 674517 [details] [diff] [review]
All NSS patches rolled up into one

Review of attachment 674517 [details] [diff] [review]:
-----------------------------------------------------------------

Brian: I don't know how to review the rest of the patch.
Do you have a script that I can run?

In particular, I saw some int64's changed to PRTime's.
That is hard to do with a script. How was that done?

Thanks.

Comment 34

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Wan-Teh Chang from comment #32)
> Brian: the only change necessary for mozilla/dbm and mozilla/security/dbm
> is my mcom_db.h change in attachment 673491 [details] [diff] [review]. It is
> not necessary to change 'uint' to 'unsigned int'. Thanks.

OK. I can revert this change. However, how do you know that it is safe to typedef PRUintn uint after including sys/types? What if sys/types includes a typedef (not a #define) for uint?

Comment 35

[Isaac Aggrey [:isaac]]

Attachment: Script to convert old prarena.h to plarena.h types

This conversion script corresponds to the macro definitions here: https://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/obsolete/protypes.h#149

Note: By MXR, the only obsolete macros still in use are PRArenaPool, PR_ARENA_ALLOCATE, and PL_ARENA_RELEASE; however, the latter two only have two and one use(s) in the code, respectively, so using the script for those two is a bit overkill.

Comment 36

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Isaac's script from bug 794510, modified for this purpose

Wan-Teh, I used my text editor's recursive search and recursive search-and-replace features to verify Isaac's changes.

The key to reviewing the int64 -> PRTime change is to first find all the uses of int64 in NSS, and verify that they should all be PRTime. I.e. we shouldn't need to use PRInt64 anywhere. I used:

find mozilla/security/nss -name "*.c" -o -name "*.h" | xargs grep -E "\\bint64\\b"

(Perhaps the grep "-C" context option helps.)

You should be able to run the script I am attaching against a clean tree, and then apply the roll-up patch to another clean tree, and then review the recursive diff between the two trees to catch some of the manual tweaks. Note that I actually have not tested this script because the version of sed I have on Windows (from mozilla-build) is horrible. (That's why I used VS's recursive search and replace.) So, you may need to tweak it.Also, I didn't try to handle the "PR_PUBLIC_API(extern "-> "PR_EXTERN(" convesion with this script, since most/all of those changes have additional whitespace changes too.

Also, I think that if you have patches that would get bitrotted by this change, you should be able to run this script on your patch files (with modified "-iname" parameters to find(1) to automatically fix your patches so they will continue to apply.

Comment 37

[Isaac Aggrey [:isaac]]

Comment on attachment 674911 [details]
Script to convert old prarena.h to plarena.h types

Ignore. Brian's script has the whole thing. ;-)

Comment 38

[Wan-Teh Chang]

(In reply to Brian Smith (:bsmith) from comment #34)
> ... However, how do you know that it is safe to
> typedef PRUintn uint after including sys/types? What if sys/types includes a
> typedef (not a #define) for uint?

I see... I didn't notice the uint typedef in "obsolete/protypes.h" is inside
an ifdef:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/include/obsolete/protypes.h&rev=3.25&mark=59-61#55

To be safe, let's replicate that ifdef in mcom_db.h. (Omit !defined(XP_BEOS)
because BeOS is not supported.) See also bug 493378.

Note: we may have problem with the int32 typedef on AIX and HP-UX:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/include/obsolete/protypes.h&rev=3.25&mark=111-112#111

Since we don't have access to those two platforms, I'll just wait for bug
reports.

Comment 39

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Allow NSS to be built with NO_NSPR_10_SUPPORT

Wan-Teh,

I rebased the patches, undid my changes to lib/dbm and made the change you suggested, copying the #ifdef'd definition of uint. See the comments above for some suggestions on how to review this large patch; in particular, see my note about s/int64/PRTime/.

Comment 40

[Wan-Teh Chang]

Comment on attachment 744975 [details] [diff] [review]
Allow NSS to be built with NO_NSPR_10_SUPPORT

Review of attachment 744975 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. Please check this in ASAP. I am surprised to see some
manual adjustments to maintain good code formatting. Quite
impressive.

::: lib/dbm/include/mcom_db.h
@@ +46,5 @@
> +typedef PRUintn uint;
> +#endif
> +typedef PRUint8 uint8;
> +typedef PRUint16 uint16;
> +typedef PRInt32 int32;

There is a CR character at the end of these three lines. In vim,
you can find them by doing /\r

Please put this block of code between #include "prtypes.h" and
#include <limits.h>.

::: lib/freebl/unix_rand.c
@@ -600,5 @@
>  
>  static size_t
>  GetHighResClock(void *buf, size_t maxbytes)
>  {
> -    bigtime_t bigtime; /* Actually an int64 */

Don't change this comment because this is talking about the BeOS
bigtime_t type.

::: lib/jar/jzlib.h
@@ +680,3 @@
>  				 const Bytef *source, uLong sourceLen);
>  #else
> +extern int EXPORT uncompress OF((Bytef *dest,  uLongf *destLen,

It seems that we should leave this line unchanged, and align the
"uLongf *destLen" argument in the #ifdef MOZILLA_CLIENT part to
match.

::: lib/util/dertime.c
@@ +46,5 @@
>      if (!d) {
>  	return SECFailure;
>      }
>  
> +    /* Convert a PRTime time to a printable format.  */

"a PRTime time" => "a PRTime" ?

There is another instance of this. You can search for "a PRTime time"
in the patch to find it.

Comment 41

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Wan-Teh Chang from comment #40)
> ::: lib/jar/jzlib.h
> @@ +680,3 @@
> >  				 const Bytef *source, uLong sourceLen);
> >  #else
> > +extern int EXPORT uncompress OF((Bytef *dest,  uLongf *destLen,
> 
> It seems that we should leave this line unchanged, and align the
> "uLongf *destLen" argument in the #ifdef MOZILLA_CLIENT part to
> match.

I meant to undo this change, but I messed up and left it in. It will make the history noisy but I think it is a better change anyway.

> ::: lib/util/dertime.c
> @@ +46,5 @@
> >      if (!d) {
> >  	return SECFailure;
> >      }
> >  
> > +    /* Convert a PRTime time to a printable format.  */
> 
> "a PRTime time" => "a PRTime" ?
> 
> There is another instance of this. You can search for "a PRTime time"
> in the patch to find it.

There was a third instance of this, that wasn't added by this patch, that I also fixed.

Also, Isaac, I am sorry that I forgot to credit you in the hg commit message--a big omission on my part. Thank you for doing this work.

https://hg.mozilla.org/projects/nss/rev/0b69d6cc3acd

Comment 42

[Isaac Aggrey [:isaac]]

(In reply to Brian Smith (:bsmith) from comment #41)
> Also, Isaac, I am sorry that I forgot to credit you in the hg commit
> message--a big omission on my part. Thank you for doing this work.

It was my pleasure, and I'm looking forward to contribute more in the near future after I graduate (woo!). I honestly would not have even noticed, but I really do appreciate the careful stance you take towards attribution, Brian. Thanks.