Use bitfields in nsHTMLInputElement

RESOLVED FIXED in mozilla0.9.9

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: John Keiser (jkeiser), Assigned: John Keiser (jkeiser))

Tracking

Trunk
mozilla0.9.9
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

6.65 KB, patch
Brian Ryner (not reading)
: review+
Details | Diff | Splinter Review
6.91 KB, patch
Brian Ryner (not reading)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

16 years ago
Use an int with bits on/off for nsHTMLInputElement's myriad PRPackedBools.
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
Depends on: 108307
Target Milestone: --- → mozilla0.9.9
(Assignee)

Comment 1

16 years ago
Created attachment 67758 [details] [diff] [review]
Patch
(Assignee)

Comment 2

16 years ago
Bitfield!  Yay!
Some people would probably disagree with me, but I think that hard coding member
variable names into macros is not a good idea, it hides things behind the macros
and IMO makes the code harder to understand. I'd suggest (feel free to disagree)
that you add a parameter to the macros and always pass in the bits that the
macros operate on. Let me know which way you want to go and I'll sr...
(Assignee)

Comment 4

16 years ago
OK, I think I agree with you.  It would at least make it clear to have a
GET_BIT_STATE macro instead of getting individual fields.  You have to spend
time otherwise thinking, "why the hell did they make a macro for that?"
(Assignee)

Comment 5

16 years ago
Created attachment 67789 [details] [diff] [review]
Patch v1.1

Use GET_BOOLBIT and SET_BOOLBIT macros, and fix bug where it would only turn
bits on, not off :)  Tested against checkbox and radio button.
Comment on attachment 67789 [details] [diff] [review]
Patch v1.1

r=bryner
Attachment #67789 - Flags: review+
Hmm, there's also:

  http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/include/prbit.h#46

Could we use those in stead of rewriting these macros? Sorry for going back and
fourth on this...
(Assignee)

Comment 8

16 years ago
Created attachment 67876 [details] [diff] [review]
Patch v1.2

Make bit setters/getters more generic
Comment on attachment 67876 [details] [diff] [review]
Patch v1.2

Nit of the day:

+#define GET_BOOLBIT(bitfield,field) (bitfield & (0x01 << (field)))
+#define SET_BOOLBIT(bitfield,field,b) ((b) ? (bitfield |=  (0x01 << (field)))
\
+					    : (bitfield &= ~(0x01 << (field))))

Add space after comma (',').

Also, GET_BOOLBIT() needs to make sure it always returns either 0 or 1, the
current macro can return a value with any one bit set, or with no bit set. This
will make GET_BOOLBIT(...) == PR_TRUE be false even if the bit is set. I.e. add
a ? 1 : 0; at the end of the macro to avoid problems with this in the future.

sr=jst with that fixed.
Attachment #67876 - Flags: superreview+
Attachment #67876 - Flags: review+
Comment on attachment 67876 [details] [diff] [review]
Patch v1.2

r=bryner
Why not use C's bitfields feature here? As in:
class nsHTMLInputElement {
...
PRPackedBool value1 : 1;
PRPackedBool value2 : 1;
PRPackedBool value3 : 1;
...
};

That would make accessing the values much easier.
Yeah, sure, that would work too. I guess those are potentially dangerous when
assigning into them though, AFAIK assigning a value that doesn't have the low
bit set (but has other bits set) into a 1-bit sized char (which is what
PRPackedBool : 1; is) will set the bit to 0, so what we have here is less error
prone...

I can't say I feel very strongly either way here...
(Assignee)

Comment 13

16 years ago
Well, I already checked it in ... but I wouldn't mind using that syntax.  I'll
keep it in the back of my head for next time.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Updated

16 years ago
QA Contact: madhur → tpreston
You need to log in before you can comment on or make changes to this bug.