Closed
Bug 1342330
Opened 7 years ago
Closed 7 years ago
Refine type declarations for fields in nsLineBox::FlagBits
Categories
(Core :: Layout: Block and Inline, defect)
Core
Layout: Block and Inline
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: TYLin, Assigned: TYLin)
References
Details
Attachments
(3 files)
We should use bool for those 1 bit flags and StyleClear for mBreakType. http://searchfox.org/mozilla-central/rev/60ae6514e4c559c0c234f0e7aefccb101b8beb2e/layout/generic/nsLineBox.h#659-680
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
Oops. I got the compile error on Linux because I declared "StyleClear mBreakType : 4;" error: 'nsLineBox::FlagBits::mBreakType' is too small to hold all values of 'using StyleClear = enum class mozilla::StyleClear {aka enum class mozilla::StyleClear}'
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8840760 [details] Bug 1342330 Part 1 - Strip trailing whitespaces in nsLineBox.h. https://reviewboard.mozilla.org/r/115180/#review116676
Attachment #8840760 -
Flags: review?(xidorn+moz) → review+
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8840761 [details] Bug 1342330 Part 2 - Convert uint32_t to bool in nsLineBox::FlagBits. https://reviewboard.mozilla.org/r/115182/#review116678 ::: layout/generic/nsLineBox.h:679 (Diff revision 1) > - uint32_t mHasBullet : 1; > + bool mHasBullet : 1; > // Indicates that this line *may* have a placeholder for a float > // that was pushed to a later column or page. > - uint32_t mHadFloatPushed : 1; > - uint32_t mHasHashedFrames: 1; > + bool mHadFloatPushed : 1; > + bool mHasHashedFrames: 1; > uint32_t mBreakType : 4; This should be changed to `uint8_t` here, since otherwise you are actually making this struct larger. But it seems you are changing this to an enum with its underlying type being `uint8_t` in the next patch... so it's probably not really an issue here.
Attachment #8840761 -
Flags: review?(xidorn+moz) → review+
Assignee | ||
Comment 7•7 years ago
|
||
I found that we have a assertion here to ensure FlagBits is smaller than uint32_t. I guess in Part 3, I could omit the ":4" to fix the compile error in comment 4 without exceeding the limit. http://searchfox.org/mozilla-central/rev/60ae6514e4c559c0c234f0e7aefccb101b8beb2e/layout/generic/nsLineBox.h#705-706
Comment 8•7 years ago
|
||
That looks like a GCC bug...
Comment 9•7 years ago
|
||
I would suggest we just disable that warning... since that doesn't seem to be anything useful... GCC checks whether bit field is big enough based on the underlying type of an enum, while clang doesn't check this at all... But... this warning cannot be disabled separately... Then, either changing it to uint8_t or making it not a bitfield sounds fine to me.
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8840762 [details] Bug 1342330 Part 3 - Convert FlagBits::mBreakType to StyleClear. https://reviewboard.mozilla.org/r/115184/#review116696 Cancel for now since the bustage.
Attachment #8840762 -
Flags: review?(xidorn+moz)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
Try results for patchset 2. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d4b42f5aff024abc59130d930f6e5b666ccd252
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8840762 [details] Bug 1342330 Part 3 - Convert FlagBits::mBreakType to StyleClear. https://reviewboard.mozilla.org/r/115184/#review116734
Attachment #8840762 -
Flags: review?(xidorn+moz) → review+
Comment 14•7 years ago
|
||
Pushed by tlin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bea0382ab854 Part 1 - Strip trailing whitespaces in nsLineBox.h. r=xidorn https://hg.mozilla.org/integration/autoland/rev/55a43be33799 Part 2 - Convert uint32_t to bool in nsLineBox::FlagBits. r=xidorn https://hg.mozilla.org/integration/autoland/rev/94dabc6479b2 Part 3 - Convert FlagBits::mBreakType to StyleClear. r=xidorn
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bea0382ab854 https://hg.mozilla.org/mozilla-central/rev/55a43be33799 https://hg.mozilla.org/mozilla-central/rev/94dabc6479b2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•