795511 - Remove usages of PR_BIT and PR_BITMASK from our tree

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[(no longer active)]

PR_BIT(x) can be replaced with 0x1 << x.

Comment 1

[(no longer active)]

PR_BITMASK should be easy too:

(0x1 << x) - 1

Comment 2

[Abhishek Potnis [:avp]]

Hi,

I would like to work on this bug. Could you please guide me to get started on it ? Which
must be the files that need to be edited ?


Thanks.

Comment 3

[(no longer active)]

The guidelines here are quite similar to bug 795504 comment 2.

Please let me know if you need help.  Thanks!

Comment 4

[Joshua Cranmer [:jcranmer]]

(In reply to Ehsan Akhgari [:ehsan] from comment #0)
> PR_BIT(x) can be replaced with 0x1 << x.

One word of caution: 0x1U (or (uint32_t)1) is necessary to keep the expression from being undefined if bit 31 is to be set (Similarly for PR_BITMASK). There may also be cases where PR_BIT/PR_BITMASK is relied on to be an unsigned integer instead of a signed integer.

Comment 5

[Masatoshi Kimura [:emk]]

1u << x would be the most concise.

Comment 6

[(no longer active)]

(In reply to comment #5)
> 1u << x would be the most concise.

True!

Comment 7

[Abhishek Potnis [:avp]]

So should I replace  PR_BIT with 1u << x and PR_BITMASK with (1u << x)-1 ?

Comment 8

[(no longer active)]

(In reply to comment #7)
> So should I replace  PR_BIT with 1u << x and PR_BITMASK with (1u << x)-1 ?

Sure.

Comment 9

[Abhishek Potnis [:avp]]

Attachment: Removed usages of PR_BIT and PR_BITMASK from mozilla-central tree

Comment 10

[Masatoshi Kimura [:emk]]

Comment on attachment 673174 [details] [diff] [review]
Removed usages of PR_BIT and PR_BITMASK from mozilla-central tree

>+#define DELAY_LINE_LENGTH_MASK  (1u << DELAY_LINE_LENGTH_LOG2) - 1
>+#define DELAY_LINE_LENGTH       1u << DELAY_LINE_LENGTH_LOG2
Surround it with parenthesis. (Consider about how DELAY_LINE_LENGTH + 1 will be expanded.)

Comment 11

[(no longer active)]

Comment on attachment 673174 [details] [diff] [review]
Removed usages of PR_BIT and PR_BITMASK from mozilla-central tree

Review of attachment 673174 [details] [diff] [review]:
-----------------------------------------------------------------

Looks very good, thanks Abhishek!  Please address the following comments and attach another version of the patch for review.

::: xpcom/io/nsEscape.h
@@ +21,5 @@
>  typedef enum {
>   	url_All       = 0         /**< %-escape every byte unconditionally */
> +,	url_XAlphas   = 1u << 0 /**< Normal escape - leave alphas intact, escape the rest */
> +,	url_XPAlphas  = 1u << 1 /**< As url_XAlphas, but convert spaces (0x20) to '+' and plus to %2B */
> +,	url_Path      = 1u << 2 /**< As url_XAlphas, but don't escape slash ('/') */

Nit: please fix the indentation of the comments here.

::: xpcom/threads/TimerThread.h
@@ +72,5 @@
>    nsTArray<nsTimerImpl*> mTimers;
>  
>  #define DELAY_LINE_LENGTH_LOG2  5
> +#define DELAY_LINE_LENGTH_MASK  (1u << DELAY_LINE_LENGTH_LOG2) - 1
> +#define DELAY_LINE_LENGTH       1u << DELAY_LINE_LENGTH_LOG2

What Masatoshi said.  :-)

Comment 12

[Abhishek Potnis [:avp]]

Attachment: Removed usages of PR_BIT and PR_BITMASK from mozilla-central tree

Comment 13

[(no longer active)]

Comment on attachment 673506 [details] [diff] [review]
Removed usages of PR_BIT and PR_BITMASK from mozilla-central tree

Review of attachment 673506 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thanks!

Comment 14

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67b6f14cd9fb

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/67b6f14cd9fb