We need something like C's INT_MAX for NSPR's PRInt32, PRUint32, etc.

for some future release.

Unlike C's int, which does not have a fixed size, NSPR's PRInt32 is 32-bit. So something like PR_INT32_MAX is not that useful. How would you use PR_INT32_MAX?

Reassign to wtc.

it's a lot more readable than 0x7fffffff

PR_UINT32_MAX is useful too, much better than PRUInt32(-1).

This oughta do it.

Assigning to me so I can remember to follow up

Comment on attachment 89352 [details] [diff] [review] Patch Joe, Sorry it took me so long to review your patch. 1. I am wondering why you don't use -0x80, -0x8000, and -0x80000000 in these definitions. >+#define PRINT8_MIN (-0x7f - 1) >+#define PRINT16_MIN (-0x7fff - 1) >+#define PRINT32_MIN (-0x7fffffff - 1) 2. For the 32-bit limits, you should use the PR_INT32 and PR_UINT32 macros (see prtypes.h) so that they have the right suffices. 3. These 64-bit constants do not meet NSPR's requirement to support compilers that don't have a 64-bit integer type. Under those compilers, PRInt64 and PRUint64 are implemented as a struct. >+#define PRINT64_MIN (-0x7fffffffffffffff - 1) >+#define PRINT64_MAX 0x7fffffffffffffff >+#define PRUINT64_MIN 0 >+#define PRUINT64_MAX 0xffffffffffffffff Note that we already have the LL_MaxInt and LL_MinInt functions that return the maximum and minimum values of PRInt64. (See http://www.mozilla.org/projects/nspr/reference/html/prlong.html#21990.) There are no functions that return the maximum and minimum values of the unsigned PRUint64 type though.

John, I should have looked up your name in the Phonebook before I wrote my comment. :-) Sorry.

I didn't use the negatives because I was concerned about compilers that might first overflow 0x80000000 as a negative number (-0?), and then try to reverse it to positive with undefined results. But since we apparently assume it in the LL_MinInt function, I'll assume it's safe :)

Fixes the above issues and adds a LL_MaxUInt method and LL_MAXUINT and LL_MINUINT macros to prlong.h.

Tested all constants and they work--there was a problem with PRINT64_MIN, it referenced a nonexistent function.

wtc is taking over to do some more mods to the patch. wtc, I'm happy to review or help in any way. It'd be nice to have this.

Comment on attachment 91529 [details] [diff] [review] Patch v1.1.1 Sorry about the late review. Part of the reason is that it is not easy to define these macros correctly. For example, the following actually defines an unsigned constant: >+#define PRINT8_MIN 0x80 You can verify that by the following test: int i; i = 0x80; printf("%d\n", i); You will see that 128, instead of -128, is printed.

Changes from the v1.1.1 patch. 1. No macros are defined for the minimum values of the unsigned types because they are 0. 2. Use PR_INT8_MAX instead PRINT8_MAX, and similarly for the other macros. 3. I did not define the macros for the maximum and minimum values of PRInt64 and PRUint64. 4. The definition of PR_INT32_MIN looks funny, but is how INT_MIN is defined in <limits.h> on AIX, HP-UX, Linux, Solaris, and OSF1. I don't know why but I believe there must be a good reason for that.

Comment on attachment 99615 [details] Patch v2 PR_INT32_MIN really should be PR_INT32(-2147483648) for consistency unless there is a *need* to do this funkiness. Just because the other file does it doesn't mean this one should too :) r=jkeiser with that, if I am allowed to review in this file :) I guess I'm OK with getting rid of the UINT_MIN's since everyone knows they are 0. But I am curious why we can't do the PRUint64 min/max stuff? If we use the functions at least we know it will work like it should in all cases and have a consistent naming convention. Anyway, your module, your call :)

Comment on attachment 99615 [details] Patch v2 John, thank you for reviewing this patch. You are welcome to review NSPR patches! All the <limit.h> files I looked at define INT_MIN like this: #define INT_MIN (-INTMAX - 1) And similarly for LONG_MIN. One of these <limit.h> files has a comment that says "define in sneaky way so result type is int". I still don't know why, but just to be on the safe side I decided to do the same thing. Perhaps someone like Brendan can explain to us why the funkiness is necessary. I will look at the 64-bit macros later. It's difficult to construct tests for these macros so I need to proceed very carefully.

Comment on attachment 99615 [details] Patch v2 I found that if I define PR_INT32_MIN the obvious way: #define PR_INT32_MIN PR_INT32(-2147483648) it turns out to be a 'long'; sizeof(PR_INT32_MIN) is 8 on OSF1 and 64-bit Solaris where 'long' is 8 bytes. This experiment shows that the funkiness in the definition is necessary, although I still don't know why.

Differences from the previous patch (attachment 91529 [details] [diff] [review]) are: 1. LL_MaxUint (note the spelling of 'Uint') returns PRUint64 instead of PRInt64. 2. There is no macro or function for the minimum value for PRUint64 because it is zero. 3. I did not define PR_INT64_MAX, PR_INT64_MIN, and PR_UINT64_MAX because they would be simply synonyms for LL_MAXINT, LL_MININT, and LL_MAXUINT. If they need to be defined, they should be defined in prlong.h instead of prtypes.h so that we don't create a dependency of prtypes.h on prlong.h.

Comment on attachment 99615 [details] Patch v2 I've checked in this patch on the tip and NSPRPUB_PRE_4_2_CLIENT_BRANCH of NSPR.

Comment on attachment 99672 [details] [diff] [review] Patch for unsigned 64-bit int max r=jkeiser This looks good to me also, I can't believe I made the return value be PRInt64 :) Looking at PRInt64, you are right--we're apparently supposed to use macros for all ops on PRInt64 so it's fundamentally different from the other PRInt*'s. I question whether it should be named PRInt64 if it has a different interface than the other ints though--and I'll bet you we use PRInt64's in the current codebase as though they are simple int types (using operators directly). Anyway, LL_* is fine.

John, In the 1999 revision of the C standard, the 'long long' type was officially added. At present I don't know of any compiler we are using that doesn't support a 64-bit integer type (which isn't necessarily called 'long long'). I expect that in a few years C99 support will become widespread and there will be no need to "fix" the code that uses operators directly on PRInt64.

In that case, shouldn't we make 64-bit ints have the same interface as 32-bit? PRINT64_MAX, even if it is a synonym of LL_MAX, is a lot more discoverable than LL_MAX since it's an analogue of what people already know. It is also more consistent. The only reason I could see to only do LL_MAX and not PRINT64_MAX is if PRInt64 was fundamentally, significantly different from PRInt32 from a user's point of view. If that's not the case, I think PRINT64_MAX should really be defined.

Colin, I need to add a new symbol LL_MaxUint to NSPR 4.3. Could you come up with a patch for nsprpub/pr/src/nspr_symvec.opt? It seems to me that LL_MaxUint can take the place of PR_VMS_Stub54 and then we can delete PR_VMS_Stub54.

Wan-Teh, you're right, we can start using some of the stub positions in the symbol vector. The patch uses Stub54's position for LL_MaxUint. Note that I have not been able to test this patch since the NSPR which ships with Mozilla 1.3 doesn't include the new LL_MaxUint work. And for the same reason I haven't removed the Stub54 code yet either.If you want me to complete the work and test it'll be next week.

Colin, thanks for the OpenVMS patch. I'm going to mark this bug fixed in NSPR 4.3. I will open a new NSPR bug for the addition of LL_MaxUint, which I haven't done.

Comment on attachment 99672 [details] [diff] [review] Patch for unsigned 64-bit int max This patch has been moved to the new bug on LL_MaxUint (Bug 195411).

Comment on attachment 115856 [details] [diff] [review] OpenVMS patch for use of LL_MaxUint This patch has been moved to the new bug on LL_MaxUint (Bug 195411).