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.
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). Thanks!
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
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: https://tbpl.mozilla.org/?tree=Try&rev=f5b7df674d03 I've verified that any problems are intermittent.
Checking in pr/include/prtypes.h; /cvsroot/mozilla/nsprpub/pr/include/prtypes.h,v <-- prtypes.h new revision: 3.47; previous revision: 3.46 done
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Thank you for the patch. I'd like to suggest some coding style changes. Please remove the comment "In any case, ..." and the macro definitions for the !HAVE_LONG_LONG case (unless you have tested them): http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/include/prtypes.h&rev=3.47&mark=381-382,395-399#381 It is fine for the !HAVE_LONG_LONG case to become out of date because all compilers support a 64-bit integer type today. Please replace the comment I asked you to delete with a comment that describes what the macros are. Model it after PR_INT32_MAX, etc.: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/include/prtypes.h&rev=3.47&mark=324-335#324 Thank you.
Status: RESOLVED → REOPENED
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.
Merged to m-c: https://hg.mozilla.org/mozilla-central/rev/765f5b79fedf
I just remember that these constants already exist in NSPR as both functions and macros: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/include/prlong.h&rev=3.16&mark=62-63,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.
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 done
Severity: normal → enhancement
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Priority: -- → P2
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.