Closed
Bug 776785
Opened 12 years ago
Closed 12 years ago
Alarm API - hal::SetAlarm takes |long|s, but should instead take PRInt32s
Categories
(Core :: Hardware Abstraction Layer (HAL), defect)
Core
Hardware Abstraction Layer (HAL)
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: justin.lebar+bug, Assigned: airpingu)
References
Details
Attachments
(1 file)
4.04 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
>+bool SetAlarm(long aSeconds, long aNanoseconds)
These should both be PRInt32's, as far as I can tell. The only caller, AlarmHalService, has PRInt32s.
We avoid raw types in Gecko code because the meaning of |long| is different on different platforms. (It's 64 bits wide on Linux 64, and 32 bits wide on Linux 32 and Windows 64.)
Is there a good reason that these are long's? If not, we should fix it.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → clian
Assignee | ||
Updated•12 years ago
|
Summary: hal::SetAlarm takes |long|s, but should instead take PRInt32s → Alarm API - hal::SetAlarm takes |long|s, but should instead take PRInt32s
Reporter | ||
Comment 1•12 years ago
|
||
Ping on this bug. I can review the fix if you write the patch, Gene.
Assignee | ||
Comment 2•12 years ago
|
||
Thanks Justin! :) Sorry that I was busy with other issues. I'll come back with the patch today ASAP. Please stay tuned.
Assignee | ||
Comment 3•12 years ago
|
||
Hi Justin, Changes are trivial: replace the |long|s by |PRInt32|s. In addition to the hal:: functions, I also did the same thing for IDL for consistency. Please feel free to let me know if you didn't agree with these changes. Thanks for your reviews in advance :)
Attachment #647414 -
Flags: review?(justin.lebar+bug)
Reporter | ||
Comment 4•12 years ago
|
||
Comment on attachment 647414 [details] [diff] [review] Patch Looks great; thanks! Do you need me to check this in for you?
Attachment #647414 -
Flags: review?(justin.lebar+bug) → review+
Assignee | ||
Comment 5•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #4) > Comment on attachment 647414 [details] [diff] [review] > Patch > > Looks great; thanks! > > Do you need me to check this in for you? Yes, thanks Justin! :) I don't have the privilege to check it in. Not yet pass the try sever (https://tbpl.mozilla.org/?tree=Try&rev=eb38577cd8d4) but I believe it works fine, since all the builds are successful and these changes won't affect the testing results. ;)
Flags: in-testsuite-
Keywords: checkin-needed
Reporter | ||
Comment 6•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcf4a3c07234
Reporter | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 7•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bcf4a3c07234
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment 8•12 years ago
|
||
Note that we do use 'long' in IDL; the changes to IDL files in this bug were inappropriate.
Reporter | ||
Comment 9•12 years ago
|
||
(In reply to :Ms2ger (Back and backlogged) from comment #8) > Note that we do use 'long' in IDL; the changes to IDL files in this bug were > inappropriate. We also use PRInt32/PRUint32. Is that deprecated for some bizarre reason? $ git ls-files '*.idl' | xargs grep PRUint32 | wc -l 360 $ git ls-files '*.idl' | xargs grep PRInt32 | wc -l 102
You need to log in
before you can comment on or make changes to this bug.
Description
•