Closed Bug 1342330 Opened 3 years ago Closed 3 years ago

Refine type declarations for fields in nsLineBox::FlagBits

Categories

(Core :: Layout: Block and Inline, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

Details

Attachments

(3 files)

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 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 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+
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
That looks like a GCC bug...
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 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 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+
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
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: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.