Closed Bug 795511 Opened 7 years ago Closed 7 years ago

Remove usages of PR_BIT and PR_BITMASK from our tree

Categories

(Core :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: ehsan, Assigned: avp)

References

(Blocks 1 open bug)

Details

(Whiteboard: [mentor=ehsan][lang=c++])

Attachments

(1 file, 1 obsolete file)

PR_BIT(x) can be replaced with 0x1 << x.
PR_BITMASK should be easy too:

(0x1 << x) - 1
Summary: Remove usages of PR_BIT from our tree → Remove usages of PR_BIT and PR_BITMASK from our tree
Whiteboard: [mentor=ehsan][lang=c++]
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.
The guidelines here are quite similar to bug 795504 comment 2.

Please let me know if you need help.  Thanks!
Assignee: nobody → abhishekp.bugzilla
(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.
1u << x would be the most concise.
(In reply to comment #5)
> 1u << x would be the most concise.

True!
Status: NEW → ASSIGNED
So should I replace  PR_BIT with 1u << x and PR_BITMASK with (1u << x)-1 ?
(In reply to comment #7)
> So should I replace  PR_BIT with 1u << x and PR_BITMASK with (1u << x)-1 ?

Sure.
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 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.  :-)
Attachment #673174 - Flags: feedback?(ehsan) → feedback+
Attachment #673174 - Attachment is obsolete: true
Attachment #673506 - Flags: review?(ehsan)
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!
Attachment #673506 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/67b6f14cd9fb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.