Closed Bug 121317 Opened 23 years ago Closed 22 years ago

Use bitfields in nsHTMLInputElement

Categories

(Core :: Layout: Form Controls, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla0.9.9

People

(Reporter: john, Assigned: john)

References

Details

Attachments

(2 files, 1 obsolete file)

Use an int with bits on/off for nsHTMLInputElement's myriad PRPackedBools.
Status: NEW → ASSIGNED
Depends on: 108307
Target Milestone: --- → mozilla0.9.9
Attached patch Patch (obsolete) — Splinter Review
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...
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?"
Attached patch Patch v1.1Splinter Review
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...
Attached patch Patch v1.2Splinter Review
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...
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: 22 years ago
Resolution: --- → FIXED
QA Contact: madhur → tpreston
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: