Closed Bug 123403 Opened 22 years ago Closed 20 years ago
_INIT should use PR _UINT32, not PR _INT32
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
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
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"
Attachment #147132 - Flags: review?(wchang0222) → review+
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.
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);
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
Closed: 20 years ago
Resolution: --- → FIXED
Target Milestone: 4.2 → 4.6
"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.