Closed Bug 674277 Opened 12 years ago Closed 11 years ago

No PR macro for maximum 64 bit value


(NSPR :: NSPR, enhancement, P2)



(Not tracked)



(Reporter: jwir3, Assigned: jwir3)




(2 files, 3 obsolete files)

Every PRInt* and PRUint* type has a corresponding PR_UINT*_MAX macro, except for PRUint64. It would be nice if we had a convenience macro for this in prtypes.h as well.
Attached patch Patch (v1) (obsolete) — Splinter Review
Hi Benjamin: 
I was told you were the correct person to review this small piece of code.

I believe I did this correctly. Please let me know if I missed something, or if this isn't the correct way to implement the macro. (Or if you're not the correct person to review it).

Assignee: nobody → sjohnson
Attachment #566072 - Flags: review?(benjamin)
Comment on attachment 566072 [details] [diff] [review]
Patch (v1)

I am not an NSPR peer, so you need to request review from either wtc@google or :ted.

But I'm pretty sure there's a typo here in PR_INT64_MIN where you have PR_INT54_MAX?

Also I don't this will be accepted, because NSPR still supports systems that do not have a 64-bit integral type, hence all the HAVE_LONG_LONG ifdefs above this code.
Attachment #566072 - Flags: review?(benjamin) → review-
Attachment #566072 - Attachment is obsolete: true
Attached patch Patch (v2) (obsolete) — Splinter Review
Attachment #566843 - Flags: review?(ted.mielczarek)
Comment on attachment 566843 [details] [diff] [review]
Patch (v2)

Review of attachment 566843 [details] [diff] [review]:

This looks totally reasonable to me.
Attachment #566843 - Flags: review?(ted.mielczarek) → review+
Component: General → NSPR
Product: Core → NSPR
QA Contact: general → nspr
Target Milestone: --- → 4.9
Version: Trunk → 4.9
Just rebased the patch. Also verified on try server:

I've verified that any problems are intermittent.
Attachment #566843 - Attachment is obsolete: true
Attachment #568252 - Flags: review+
Keywords: checkin-needed
Checking in pr/include/prtypes.h;
/cvsroot/mozilla/nsprpub/pr/include/prtypes.h,v  <--  prtypes.h
new revision: 3.47; previous revision: 3.46
Closed: 11 years ago
Resolution: --- → FIXED
Thank you for the patch.  I'd like to suggest some coding style

Please remove the comment "In any case, ..." and the macro
definitions for the !HAVE_LONG_LONG case (unless you have tested

It is fine for the !HAVE_LONG_LONG case to become out of
date because all compilers support a 64-bit integer type

Please replace the comment I asked you to delete with a comment
that describes what the macros are.  Model it after PR_INT32_MAX,

Thank you.
Resolution: FIXED → ---
(In reply to Wan-Teh Chang from comment #7)
> It is fine for the !HAVE_LONG_LONG case to become out of
> date because all compilers support a 64-bit integer type
> today.

So you want me to simply remove support for these macros in compilers that don't have a long long type? I did test these, by forcing the macros to use this case, even on a 64-bit machine, but I can remove them if you would rather I do that.
jwir3: thank you for testing them with HAVE_LONG_LONG undefined.
That exceeded my expectation.  You can keep them.
Ok, I'll modify the comments. I think it would be easiest if I posted this as a separate patch, given that the other patch was already applied to cvs. Unless you would prefer to revert it and have me post the whole patch for review again?
Yes, please post this as a separate patch.
Blocks: 699731
I just remember that these constants already exist in NSPR
as both functions and macros:,65,71-72,74#53

so the new macros added in this bug report are redundant.

Perhaps it's still useful to add the PR_INT64_MAX, PR_INT64_MIN,
and PR_UINT64_MAX macros because the existing macros, defined in
prlong.h, are not easy to find.
I think so. Having them in the same file, with the same naming as the other constants seems like a good thing. prlong.h seems fairly outdated since all modern toolchains have real 64-bit integer support.
Attachment #572482 - Flags: review?(wtc)
I moved Scott's comment to a different location in the file
(so that it applies to the HAVE_LONG_LONG not defined case).
I checked in the patch on the NSPR trunk (NSPR 4.9).

Checking in prtypes.h;
/cvsroot/mozilla/nsprpub/pr/include/prtypes.h,v  <--  prtypes.h
new revision: 3.48; previous revision: 3.47
Attachment #572482 - Attachment is obsolete: true
Attachment #572482 - Flags: review?(wtc)
Severity: normal → enhancement
Closed: 11 years ago11 years ago
Priority: -- → P2
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.