nsTextFragment's status bits should be unsigned

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
13 years ago
4 months ago

People

(Reporter: roc, Assigned: roc)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Posted patch fixSplinter Review
Currently nsTextFragment's FragmentBits single-bit fields are declared PRBool. This is a signed integer so when the bit is set, the value is -1. This is incorrect, all bool true values should be 1.

The fix is to declare the fields unsigned int. I'd use PRPackedBool but for some reason MSVC doesn't pack the fields correctly in that case.
Attachment #259373 - Flags: review?(Olli.Pettay)
Attachment #259373 - Flags: superreview?(jonas)
Attachment #259373 - Flags: review?(jonas)
Attachment #259373 - Flags: review?(Olli.Pettay)
Comment on attachment 259373 [details] [diff] [review]
fix

Use PRUint32 or PRUword for all 4 fields, r/sr=me with that.
Attachment #259373 - Flags: superreview?(jonas)
Attachment #259373 - Flags: superreview+
Attachment #259373 - Flags: review?(jonas)
Attachment #259373 - Flags: review+
Any chance you could update http://www.mozilla.org/hacking/portable-cpp.html with what you learned, or propose text for an update?
I don't think I have access to doctor.

What I have learned is that MSVC++ 8 (at least) doesn't seem to like mixing chars with ints when packing bitfields. So a struct like this
struct {
  char ch:1;
  int i:1;
};
will be 8 bytes. It doesn't matter whether the fields are unsigned or not, or what order they're in. If the fields are the same type (ignoring signedness), things work.

I think the best recommendation is to just require that all bitfields in a struct use the same type.
Doctor will let you create a diff of modifications to the page (even if you don't have commit access) via the Edit this Page link in the footer.

I think this covers the same-type documentation; I added the unsigned requirement since I can't think of real-world cases (when would you really need a 7-bit signed integer?) where it doesn't make sense.  If you don't like it, we can remove the last two paragraphs and change the item description easily enough.
Attachment #262409 - Flags: review?(roc)
I think we should allow signed bitfields. It could be useful. Can you revise?
(In reply to comment #5)
> I think we should allow signed bitfields. It could be useful. Can you revise?

Done, but how would they be useful?  I still can't think of non-contrived uses.

The PRBool paragraph doesn't make sense any more as written.  Does it make sense to make that a separate tip, or is it unnecessary to make it a tip?
Attachment #262409 - Attachment is obsolete: true
Attachment #264960 - Flags: review?(roc)
Attachment #262409 - Flags: review?(roc)
(In reply to comment #6)
> Done, but how would they be useful?  I still can't think of non-contrived uses.

I think I can. For example, we have some variables whose values can be -1, 0 or 1.

> The PRBool paragraph doesn't make sense any more as written.  Does it make
> sense to make that a separate tip, or is it unnecessary to make it a tip?

What PRBool paragraph? Let's handle that separately.
Attachment #264960 - Flags: superreview+
Attachment #264960 - Flags: review?(roc)
Attachment #264960 - Flags: review+
I checked in the patch.

> What PRBool paragraph? Let's handle that separately.

It's basically the note on not using PRBool for a single-bit bool because its value is -1 and not 1.  From the original patch:

+      <p>On a related note, keep in mind that since <code>PRBool</code> is
+      represented as a signed integer, you really can't use
+      <code>PRBool b : 1;</code> because if <code>b</code> is set, it will
+      be <code>-1</code>, but XPCOM requires that the Boolean true value is
+      <code>1</code>.</p>

It's basically "If you want to represent a bool value in a single bit, use an unsigned type to do so.  If you use a signed type (this includes PRBool), its value when set will be -1 instead of +1, which violates XPCOM convention.", with a title of "Use an unsigned type (not PRBool) for a bitfield which represents a boolean value."

How does this wording sound for an added tip (and again, is it evven necessary)?
marking fixed because it is.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
I added the use-unsigned-for-boolean-bitfield note to the portability guide.
Flags: in-testsuite-
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.