72100 - need something like INT_MAX for NSPR types

Status: RESOLVED FIXED

(NSPR :: NSPR, enhancement, P3)

Description

[Erik van der Poel]

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

Comment 1

[larryh (gone)]

for some future release.

Comment 2

[Wan-Teh Chang]

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?

Comment 3

[larryh (gone)]

Reassign to wtc.

Comment 4

[timeless]

it's a lot more readable than 0x7fffffff

Comment 5

[John Keiser (jkeiser)]

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

Comment 6

[John Keiser (jkeiser)]

Attachment: Patch

This oughta do it.

Comment 7

[John Keiser (jkeiser)]

Assigning to me so I can remember to follow up

Comment 8

[Wan-Teh Chang]

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.

Comment 9

[Wan-Teh Chang]

John,

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

Comment 10

[John Keiser (jkeiser)]

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 :)

Comment 11

[John Keiser (jkeiser)]

Attachment: Patch v1.1

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

Comment 12

[John Keiser (jkeiser)]

Attachment: Patch v1.1.1

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

Comment 13

[John Keiser (jkeiser)]

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 14

[Wan-Teh Chang]

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.

Comment 15

[Wan-Teh Chang]

Attachment: Patch v2

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 16

[John Keiser (jkeiser)]

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 17

[Wan-Teh Chang]

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 18

[Wan-Teh Chang]

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.

Comment 19

[Wan-Teh Chang]

Attachment: Patch for unsigned 64-bit int max

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 20

[Wan-Teh Chang]

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 21

[John Keiser (jkeiser)]

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.

Comment 22

[Wan-Teh Chang]

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.

Comment 23

[John Keiser (jkeiser)]

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.

Comment 24

[Wan-Teh Chang]

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.

Comment 25

[Colin Blake]

Attachment: OpenVMS patch for use of LL_MaxUint

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.

Comment 26

[Wan-Teh Chang]

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 27

[Wan-Teh Chang]

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 28

[Wan-Teh Chang]

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).