Closed Bug 791906 Opened 12 years ago Closed 12 years ago

Replace NSPR integer limit constants with stdint ones

Categories

(Core :: General, defect)

x86
macOS
defect
Not set
normal

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)

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!  :-)
Assignee: nobody → isaac.aggrey
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.
Attachment #665799 - Flags: review?(ehsan)
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)
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!
Attachment #665805 - Flags: review?(ehsan) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/2c694d8bf7a5
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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'
Depends on: 795584
(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.
Blocks: 796164
No longer blocks: 796164
Depends on: 797117
You need to log in before you can comment on or make changes to this bug.