LL_INIT should use PR_UINT32, not PR_INT32

RESOLVED FIXED in 4.6

Status

NSPR
NSPR
P1
trivial
RESOLVED FIXED
16 years ago
14 years ago

People

(Reporter: timeless, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

16 years ago
The destination fields are Unsigned, so we shouldn't cast them to signed in the interim.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/include/prtypes.h&rev=3.21&mark=342,344#339
(Assignee)

Comment 1

16 years ago
This is a simple change, but what is the problem?
Does some compiler warn about signed/unsigned mismatch?
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → 4.2

Comment 2

14 years ago
No compiler does not warn you about anything, but it silently does a sign
extension. 

try :

PRInt64 minus_one_a = LL_INIT(0,-1);
PRInt64 minus_one_b = LL_INIT(-1,-1);
PRInt64 minus_one_hi_and_lo = LL_INIT(0xFFFFFFFF,0xFFFFFFFF);
PRInt64 minus_one_only_lo = LL_INIT(0,0xFFFFFFFF);

/* test one if initialication is portable as it claims 
this test should pass on both systems with and without native 64 bit integers */ 
if ( LL_EQ(minus_one_a, minus_one_hi_and_lo) )
  printf "test 1 failed - (0,-1) initializes both hi and lo portions!\n" 
else
  printf "test 1 passed\n" 

/* test two if initialication is portable as it claims
this test should pass on both systems with and without native 64 bit integers */ 
if ( LL_EQ(minus_one_b, minus_one_hi_and_lo) )
  printf "test 2 passed\n" 
else
  printf "test 2 failed - (-1,-1) initialization is not working as expected!\n" 
Blocks: 241696
(Reporter)

Comment 3

14 years ago
Created attachment 147132 [details] [diff] [review]
per comment 0
(Reporter)

Updated

14 years ago
Attachment #147132 - Flags: review?(wchang0222)
(Assignee)

Updated

14 years ago
Attachment #147132 - Flags: review?(wchang0222) → review+
(Assignee)

Comment 4

14 years ago
The sign extension problem that Jan Ruzicka pointed
out in comment 2 is a different problem because it
only affects platforms where HAVE_LONG_LONG is defined.
The cause of the problem is that you cannot pass negative
integers as arguments to the LL_INIT macro.

timeless, is there any currently supported platform
where HAVE_LONG_LONG is undefined?  I am wondering
how we can test your patch.  I have already tested
it with NSPR but would like to test it with Mozilla,
too.
(In reply to comment #4)
> The cause of the problem is that you cannot pass negative
> integers as arguments to the LL_INIT macro.

How am I supposed to init a PRInt64 with -1, then?
I'd rather not do it by substracting 1 from zero...

I currently use LL_INIT(0, -1). that will obviously not work when there is no
native 64bit int type.
(Assignee)

Comment 6

14 years ago
If it is a global or static variable, you should say:

PRInt64 minus_one = LL_INIT(0xffffffff, 0xffffffff);

If it is a local variable, you should say:

    PRInt64 minus_one;

    minus_one = LL_I2L(-1);
(Assignee)

Comment 7

14 years ago
Patch checked into the NSPR trunk (NSPR 4.6) and
NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.8 alpha).

Since all currently supported platforms define
HAVE_LONG_LONG, this patch is effectively a no-op.
I did try undefining HAVE_LONG_LONG on Linux and
Solaris and building NSPR.  I didn't see any
compiler warnings before the patch was applied.
All NSPR tests (in particular lltest.c) passed
after the patch was applied.
Severity: normal → trivial
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Target Milestone: 4.2 → 4.6
(Assignee)

Comment 8

14 years ago
"no-op" is not the right word.  It should say
"this patch is effectively modifying dead code".
You need to log in before you can comment on or make changes to this bug.