Closed
Bug 791906
Opened 12 years ago
Closed 12 years ago
Replace NSPR integer limit constants with stdint ones
Categories
(Core :: General, defect)
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: ehsan.akhgari, Assigned: isaac)
References
(Blocks 1 open bug)
Details
(Whiteboard: [mentor=ehsan][lang=c++])
Attachments
(2 files, 1 obsolete file)
1014 bytes,
text/plain
|
Details | |
310.08 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
This involves modifying the script in attachment 653946 [details] to replace things like PR_INT8_MAX with INT8_MAX. This will also need us to #include <mozilla/StandardInteger.h> in places where it's not included already.
Isaac, are you interested in working on this project?
Assignee | ||
Comment 1•12 years ago
|
||
I am, thanks for the suggestion! However, I haven't had a chance to start bug 789847 that I hope to get to this weekend -- once I wrap that up, I'll probably come back and snag this bug if no one else is interested.
Reporter | ||
Comment 2•12 years ago
|
||
Sounds good, no rush! :-)
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → isaac.aggrey
Assignee | ||
Comment 3•12 years ago
|
||
Reporter | ||
Comment 4•12 years ago
|
||
Comment on attachment 665256 [details]
Script for replacing NSPR integer limit constants with stdint ones
Looks good!
Assignee | ||
Comment 5•12 years ago
|
||
A couple things: * On my Linux system (and probably others), <stdint.h> does not explicitly set some of its UINTn_MAX [1] constants as unsigned -- this causes conflicting type errors in some usage of code like NS_MIN(unsigned int, UINT16_MAX) since the constant's value is not explicitly unsigned (UINT16_MAX is 65535 vs. PR_UINT16_MAX is 65535U). See line 1986 in nsSocketTransport2.cpp: http://dxr.mozilla.org/mozilla-central/netwerk/base/src/nsSocketTransport2.cpp.html?string=nsSocketTransport2.cpp#l1986 Of course, it's possible to just cast the constant as unsigned, but is that satisfactory? I imagine there may be a more flexible way than having to cast all similar usage of UINTn_MAX constants. [2] * Should "mozilla/StandardInteger.h" be explicitly included even in instances where other headers #include it like in "mozilla/Types.h"? [1] where n is 8 or 16 [2] But for what it's worth, line 1986 of nsSocketTransport2.cpp seems to be the only spot I run into that issue; otherwise, everything appears (?) to build.
Reporter | ||
Comment 6•12 years ago
|
||
(In reply to comment #5) > A couple things: > > * On my Linux system (and probably others), <stdint.h> does not explicitly set > some of its UINTn_MAX [1] constants as unsigned -- this causes conflicting type > errors in some usage of code like NS_MIN(unsigned int, UINT16_MAX) since the > constant's value is not explicitly unsigned (UINT16_MAX is 65535 vs. > PR_UINT16_MAX is 65535U). See line 1986 in nsSocketTransport2.cpp: > http://dxr.mozilla.org/mozilla-central/netwerk/base/src/nsSocketTransport2.cpp.html?string=nsSocketTransport2.cpp#l1986 > > Of course, it's possible to just cast the constant as unsigned, but is that > satisfactory? I imagine there may be a more flexible way than having to cast > all similar usage of UINTn_MAX constants. [2] Hmm, can we fix that in StandardInteger.h? > * Should "mozilla/StandardInteger.h" be explicitly included even in instances > where other headers #include it like in "mozilla/Types.h"? No. > [1] where n is 8 or 16 > [2] But for what it's worth, line 1986 of nsSocketTransport2.cpp seems to be > the only spot I run into that issue; otherwise, everything appears (?) to > build. In that case, just doing the cast there is ok I guess.
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Ehsan Akhgari [:ehsan] from comment #6) > > Of course, it's possible to just cast the constant as unsigned, but is that > > satisfactory? I imagine there may be a more flexible way than having to cast > > all similar usage of UINTn_MAX constants. [2] > > Hmm, can we fix that in StandardInteger.h? I was wondering the same thing, but I don't think it's an option; or rather, StandardInteger.h doesn't do much else besides choose whether or not to use <stdint.h>, a Visual Studio compatible implementation, or a custom one. > > * Should "mozilla/StandardInteger.h" be explicitly included even in instances > > where other headers #include it like in "mozilla/Types.h"? > > No. Thanks, that's what I figured! > > [1] where n is 8 or 16 > > [2] But for what it's worth, line 1986 of nsSocketTransport2.cpp seems to be > > the only spot I run into that issue; otherwise, everything appears (?) to > > build. > > In that case, just doing the cast there is ok I Right, since it doesn't occur anywhere else, I was also thinking the cast is fine. An alternative is to inline the NS_MIN call like: mTimeouts[type] = (uint16_t) value > UINT16_MAX ? UINT16_MAX : value; to avoid NS_MIN's type checking. But I'm guessing for code clarity, casting is the better option. With that cleared up, patch coming soon!
Comment 8•12 years ago
|
||
What about NS_MIN<uint16_t>(value, PR_UINT16_MAX) ?
Assignee | ||
Comment 9•12 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #8) > What about > NS_MIN<uint16_t>(value, PR_UINT16_MAX) > ? We could, if we weren't trying to remove NSPR integer constants (like PR_UINT16_MAX) from the code in this bug. :-)
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #8) > What about > NS_MIN<uint16_t>(value, PR_UINT16_MAX) > ? I apologize, I read your comment too fast! I like that option actually, but since the value parameter isn't actually uint16_t would the comparison in NS_MIN still be correct? I'm not familiar with the behavior here, but we want to make sure we're still comparing the full 32-bit value to the UINT16_MAX value. In any case, I learned something today. Thanks for the suggestion, Masatoshi!
Assignee | ||
Comment 11•12 years ago
|
||
Actually, I suppose the answer to my question is to just use NS_MIN<uint32_t>(value, UINT16_MAX) then. I need to stop trying to work on bugs late at night.
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #665799 -
Flags: review?(ehsan)
Assignee | ||
Comment 13•12 years ago
|
||
mozilla-central changed a bit in the meantime, patch should apply cleanly now. Sending to tryserver again...
Attachment #665799 -
Attachment is obsolete: true
Attachment #665799 -
Flags: review?(ehsan)
Attachment #665805 -
Flags: review?(ehsan)
Comment 14•12 years ago
|
||
Try run for 8dfdb6e13502 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=8dfdb6e13502 Results (out of 16 total builds): exception: 16 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/isaac.aggrey@gmail.com-8dfdb6e13502
Comment 15•12 years ago
|
||
I am happy if LL_MAXINT and LL_MAXUINT replace with stdint type.
Comment 16•12 years ago
|
||
Try run for 07c1cf51529e is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=07c1cf51529e Results (out of 217 total builds): success: 199 warnings: 17 failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/isaac.aggrey@gmail.com-07c1cf51529e
Reporter | ||
Comment 17•12 years ago
|
||
Comment on attachment 665805 [details] [diff] [review] Replace NSPR integer limit constants with stdint ones Review of attachment 665805 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/idl/nsIScriptSecurityManager.idl @@ +270,5 @@ > out JSStackFramePtr fp); > > > const unsigned long NO_APP_ID = 0; > + const unsigned long UNKNOWN_APP_ID = 4294967295; // UINT32_MAX Ugh! yay, IDL!
Attachment #665805 -
Flags: review?(ehsan) → review+
Reporter | ||
Comment 18•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c694d8bf7a5
Target Milestone: --- → mozilla18
Reporter | ||
Comment 19•12 years ago
|
||
(In reply to Makoto Kato from comment #15) > I am happy if LL_MAXINT and LL_MAXUINT replace with stdint type. Agreed. Filed bug 795351 for that.
Comment 20•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2c694d8bf7a5
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 21•12 years ago
|
||
I'm hitting this: c:/t1/hg/comm-central/mozilla/netwerk/protocol/http/nsHttpChannel.cpp(4492) : error C2782: 'const T &mozilla::clamped(const T &,const T &,const T &)' : template parameter 'T' is ambiguous c:\t1\hg\objdir-sm\mozilla\dist\include\nsAlgorithm.h(55) : see declaration of 'mozilla::clamped' could be 'int16_t' or 'int32_t'
Reporter | ||
Comment 22•12 years ago
|
||
(In reply to comment #21) > I'm hitting this: > > c:/t1/hg/comm-central/mozilla/netwerk/protocol/http/nsHttpChannel.cpp(4492) : > error C2782: 'const T &mozilla::clamped(const T &,const T &,const T &)' : > template parameter 'T' is ambiguous > c:\t1\hg\objdir-sm\mozilla\dist\include\nsAlgorithm.h(55) : see > declaration of 'mozilla::clamped' > could be 'int16_t' > or 'int32_t' Should be fixed in bug 795584.
Reporter | ||
Updated•12 years ago
|
Blocks: dieprtypesdie
Depends on: 805372
You need to log in
before you can comment on or make changes to this bug.
Description
•