Closed
Bug 711404
Opened 13 years ago
Closed 13 years ago
PRUint64 means PRInt64 in xpconnect
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: asaf, Assigned: m_kato)
References
Details
(Keywords: dev-doc-complete)
Attachments
(2 files)
6.63 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
2.50 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
Paste this code in your Error Console: var p = Components.classes["@mozilla.org/hash-property-bag;1"].createInstance(Components.interfaces.nsIWritablePropertyBag2); p.setPropertyAsInt64 ("a", -4000); p.getPropertyAsUint64("a") Note that fixing this will break using nsIBinaryStream's read64 for signed numbers (that is, it will break my code).
Comment 1•13 years ago
|
||
So the culprit is here: http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCConvert.cpp#163 I'd be kind of curious as to why MSVC can't handle this, and what it does instead. Also, it's possible that the compiler might provide some macro we could use. Alternatively, if we care less about correctness in the top half of the range and more about protecting callers from unexpected negative numbers, we could just take the absolute value at some point. This isn't something I'm likely to work on, but I'm happy to help out anyone who needs to fix it for something.
Assignee | ||
Comment 2•13 years ago
|
||
Interesting. As long as CVS log, this code is from 1999!, so this comment is for VC6. We should test on VS2005 and VS2010.
Assignee | ||
Comment 3•13 years ago
|
||
This workaround is for VC6 and this comment issue doesn't occurs on VC2005 or later. Also, m-c doesn't support non-HAVE_LONG_LONG platform.
Assignee: nobody → m_kato
Assignee | ||
Updated•13 years ago
|
Attachment #582755 -
Flags: review?(bobbyholley+bmo)
Comment 4•13 years ago
|
||
Comment on attachment 582755 [details] [diff] [review] fix This seems reasonable. However, comment 0 indicates that it will break other things. Can you make sure those concerns are addressed before landing? r=bholley otherwise.
Attachment #582755 -
Flags: review?(bobbyholley+bmo) → review+
Assignee | ||
Comment 5•13 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #4) > Comment on attachment 582755 [details] [diff] [review] > fix > > This seems reasonable. However, comment 0 indicates that it will break other > things. Can you make sure those concerns are addressed before landing? > r=bholley otherwise. Although several IDLs use "unsigned long long" now, test is all passed. https://tbpl.mozilla.org/?tree=Try&rev=9434cd36016b If comment #0 occurs on some interfaces, IDL should use long long instead of unsigned long long, or we should remove PRUint64 support on xpidl. (PRUint64 becomes PRInt64 on current code.)
Comment 6•13 years ago
|
||
Mano, can you elaborate on how this will break things?
Reporter | ||
Comment 7•13 years ago
|
||
Well, people *could* rely on the current behavior (I /almost/ planned to do so with nsIBinaryInputStream). I cannot tell you of particular in-tree examples, nor about extensions.
Comment 8•13 years ago
|
||
(In reply to Mano from comment #7) > Well, people *could* rely on the current behavior (I /almost/ planned to do > so with nsIBinaryInputStream). I cannot tell you of particular in-tree > examples, nor about extensions. I just checked the addons MXR. There are 3 uses of nsIBinaryInputStream.read64, and all 3 use them along with read8, read16, and read32. These 3 methods _do_ return unsigned numbers, so it seems unlikely that anyone was relying on signed numbers here. Long story short - I think this can land as long as our test suite goes green, which is the case according to Makoto. Let's do it!
Comment 9•13 years ago
|
||
Might as well do this here too.
Attachment #583482 -
Flags: review?(bobbyholley+bmo)
Comment 10•13 years ago
|
||
Comment on attachment 583482 [details] [diff] [review] Part b: Remove xpc_qsDoubleToUint64 >diff --git a/dom/workers/File.cpp b/dom/workers/File.cpp There are only a few instances of PR* integer types in this file. So if you feel like switching the whole file over to <mozilla/Stdint.h> while you're at it, feel free. But you don't have to if you don't want to. r+
Attachment #583482 -
Flags: review?(bobbyholley+bmo) → review+
Comment 11•13 years ago
|
||
I don't think it makes a lot of sense to move to stdint on a per-file basis.
Comment 12•13 years ago
|
||
Makoto, I need to land some stuff tomorrow; if you don't mind, I'll land your patch as well.
Comment 13•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b59760807d25 https://hg.mozilla.org/mozilla-central/rev/f6d0b8d7ab3e
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Keywords: dev-doc-needed
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Version: unspecified → Trunk
Comment 14•12 years ago
|
||
Documentation updated: https://developer.mozilla.org/en/PRUint64 https://developer.mozilla.org/en/DOM/Blob#Gecko_notes Mentioned on Firefox 12 for developers.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•