277704 - LL_ZERO and friends should not use a function call with native ints

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

when a native 64-bit int type is available, it would be nice if LL_ZERO would
not need a function call. it can just be a #define for 0LL. similar for
LL_MAXUINT etc.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 2

[Wan-Teh Chang]

Attachment: patch v1.1

Thanks for the patch.  I moved some code around
so that we define the macros in the same order
in all cases.  The real change I made is to
define LL_MININT as an expression.  This is
how the corresponding macro LLONG_MIN is defined
on many platforms.  I don't know why it's necessary
though.  (I did the same thing with PR_INT32_MIN
in prtypes.h.)

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 170840 [details] [diff] [review]
patch v1.1

looks good to me.

Comment 4

[Wan-Teh Chang]

Nelson, I think this patch doesn't break binary
compatibility because the functions are still
there.  I'd like another pair of eyes to review
it.  Please confirm.  Thanks.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 170840 [details] [diff] [review]
patch v1.1

second review = nelson

I believe this patch is correct for all the CPU types on which NSS runs.
It's correctness on any CPU depends on the native 64-bit integer 
format being the same as the format of struct PRInt64.	I believe that
is true for all CPU types on which NSS presently runs, and is likely
to remain true.  

>+#define LL_MAXINT    9223372036854775807L
>+#define LL_MAXUINT  18446744073709551615UL

I would have preferred to see 0x7FFFFFFFFFFFFFFF instead of 92233...07L
because I could tell at a glance that the hex form is correct, and need
to use tools to verify that the decimal form is correct.  But I have
verified that now, so it's OK.

Comment 6

[Wan-Teh Chang]

Nelson, you misunderstood what the change was
because you thought PRInt64 is defined as a struct
on all platforms.

If a platform has a 64-bit integer type, PRInt64
is defined as that type.  PRInt64 is defined as
a struct only on the few platforms that don't have
a 64-bit integer type.

So the change is:
1. No change on platforms where PRInt64 is a struct.
2. On platforms where PRInt64 is a 64-bit integer type,
- LL_MAXINT was defined as a function call
- LL_MAXInt is now defined as a constant

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

No, I had no such misunderstanding.  
The issue is binary compatibility between code that uses
native PRInt64 types and code that uses the structs.
The issue is whether the values given by the new #defines 
be compatible with old code that uses the functions.
  
Imagine new code using old shared libs, or old code using new
shared libs, where the the new code treats PRInt64 as a native
type and the old code treats it as a struct.  For those 
combinations to continue to work and be compatible, it is 
necessary that the native types and the struct types place
the data in memory in an identical fashion.  I believe that 
they do that for all presently-supported CPUs.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Let me say that another way, not using the terms "old" and "new".
Code that was compiled with HAVE_LONG_LONG should be interoperable
with code that is not compiled with HAVE_LONG_LONG.  I checked to 
see if it was, and am satisfied that it is.

Comment 9

[Wan-Teh Chang]

Thanks for reviewing that.  But this patch
does not change the HAVE_LONG_LONG setting
for any platform, hence my confusion about
your review comments.