Last Comment Bug 776785 - Alarm API - hal::SetAlarm takes |long|s, but should instead take PRInt32s
: Alarm API - hal::SetAlarm takes |long|s, but should instead take PRInt32s
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Hardware Abstraction Layer (HAL) (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Gene Lian [:gene] (I already quit Mozilla)
:
Mentors:
Depends on: 780062
Blocks: alarm-api
  Show dependency treegraph
 
Reported: 2012-07-23 17:59 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-08-02 19:33 PDT (History)
5 users (show)
airpingu: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (4.04 KB, patch)
2012-07-30 20:01 PDT, Gene Lian [:gene] (I already quit Mozilla)
justin.lebar+bug: review+
Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2012-07-23 17:59:05 PDT
>+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.
Comment 1 Justin Lebar (not reading bugmail) 2012-07-30 10:01:01 PDT
Ping on this bug.  I can review the fix if you write the patch, Gene.
Comment 2 Gene Lian [:gene] (I already quit Mozilla) 2012-07-30 18:53:33 PDT
Thanks Justin! :) Sorry that I was busy with other issues. I'll come back with the patch today ASAP. Please stay tuned.
Comment 3 Gene Lian [:gene] (I already quit Mozilla) 2012-07-30 20:01:42 PDT
Created attachment 647414 [details] [diff] [review]
Patch

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 :)
Comment 4 Justin Lebar (not reading bugmail) 2012-07-30 21:14:26 PDT
Comment on attachment 647414 [details] [diff] [review]
Patch

Looks great; thanks!

Do you need me to check this in for you?
Comment 5 Gene Lian [:gene] (I already quit Mozilla) 2012-07-30 22:39:54 PDT
(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. ;)
Comment 6 Justin Lebar (not reading bugmail) 2012-07-31 06:16:22 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcf4a3c07234
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-07-31 19:19:38 PDT
https://hg.mozilla.org/mozilla-central/rev/bcf4a3c07234
Comment 8 :Ms2ger 2012-08-02 05:39:02 PDT
Note that we do use 'long' in IDL; the changes to IDL files in this bug were inappropriate.
Comment 9 Justin Lebar (not reading bugmail) 2012-08-02 06:17:21 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.