No PR macro for maximum 64 bit value

RESOLVED FIXED in 4.9

Status

P2
enhancement
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jwir3, Assigned: jwir3)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
Created attachment 566072 [details] [diff] [review]
Patch (v1)

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 2

7 years ago
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-
(Assignee)

Updated

7 years ago
Attachment #566072 - Attachment is obsolete: true
(Assignee)

Comment 3

7 years ago
Created attachment 566843 [details] [diff] [review]
Patch (v2)
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
(Assignee)

Comment 5

7 years ago
Created attachment 568252 [details] [diff] [review]
Patch (v3) [r=ted]

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.
Attachment #566843 - Attachment is obsolete: true
Attachment #568252 - Flags: review+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
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
Last Resolved: 7 years ago
Resolution: --- → FIXED
Keywords: checkin-needed

Comment 7

7 years ago
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 → ---
(Assignee)

Comment 8

7 years ago
(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.

Comment 9

7 years ago
jwir3: thank you for testing them with HAVE_LONG_LONG undefined.
That exceeded my expectation.  You can keep them.
(Assignee)

Comment 10

7 years ago
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?

Comment 11

7 years ago
Yes, please post this as a separate patch.

Comment 13

7 years ago
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.
(Assignee)

Comment 15

7 years ago
Created attachment 572482 [details] [diff] [review]
b674277-Part2 (v1) - Change of comments for new macros.
Attachment #572482 - Flags: review?(wtc)

Comment 16

7 years ago
Created attachment 597076 [details] [diff] [review]
b674277-Part2 (v2) - Change of comments for new macros. by Scott Johnson

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
Attachment #572482 - Attachment is obsolete: true
Attachment #572482 - Flags: review?(wtc)

Updated

7 years ago
Severity: normal → enhancement
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Priority: -- → P2
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.