Closed
Bug 121317
Opened 23 years ago
Closed 23 years ago
Use bitfields in nsHTMLInputElement
Categories
(Core :: Layout: Form Controls, defect)
Tracking
()
RESOLVED
FIXED
mozilla0.9.9
People
(Reporter: john, Assigned: john)
References
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
bryner
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bryner
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
Use an int with bits on/off for nsHTMLInputElement's myriad PRPackedBools.
Assignee | ||
Updated•23 years ago
|
Assignee | ||
Comment 1•23 years ago
|
||
Comment 3•23 years ago
|
||
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•23 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•23 years ago
|
||
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 6•23 years ago
|
||
Comment on attachment 67789 [details] [diff] [review] Patch v1.1 r=bryner
Attachment #67789 -
Flags: review+
Comment 7•23 years ago
|
||
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...
Comment 9•23 years ago
|
||
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+
Updated•23 years ago
|
Attachment #67876 -
Flags: review+
Comment 11•23 years ago
|
||
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.
Comment 12•23 years ago
|
||
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•23 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
Closed: 23 years ago
Resolution: --- → FIXED
Updated•23 years ago
|
QA Contact: madhur → tpreston
You need to log in
before you can comment on or make changes to this bug.
Description
•