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?

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.

Sounds good, no rush! :-)

Comment on attachment 665256 [details] Script for replacing NSPR integer limit constants with stdint ones Looks good!

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.

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

(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!

What about NS_MIN<uint16_t>(value, PR_UINT16_MAX) ?

(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. :-)

(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!

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.

mozilla-central changed a bit in the meantime, patch should apply cleanly now. Sending to tryserver again...

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

I am happy if LL_MAXINT and LL_MAXUINT replace with stdint type.

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

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!

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

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'

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