Bug 69803 – Add compile-time asserts for RenderStyle::(Non)InheritedFlags size.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Luke Macpherson]

Add compile-time asserts for RenderStyle::(Non)InheritedFlags size.

[Luke Macpherson]

Created attachment 110443 [details]
Patch

[Luke Macpherson]

Eric requested something like this on https://bugs.webkit.org/show_bug.cgi?id=64583, and it would be good to have regardless of what happens to that patch.

[David Levin]

Comment on attachment 110443 [details]
Patch

I'm probably not the person to review this, but it is clear that there is a need to fix the Windows build failure.

[Eric Seidel (no email)]

Comment on attachment 110443 [details]
Patch

Yup.  These are super useful.  I think normally we put the outside of the class itself, but this also seems OK, since it's right next to where the equals is.  You'd have to modify both.

[Luke Macpherson]

I don't know why this fails on windows, and don't have a windows build environment to test on, so I'm going to throw a few patches at the build bots and see what happens. The size really shouldn't be > 8 bytes, but maybe windows doesn't round up to a 4-byte alignment?

[Luke Macpherson]

Created attachment 110596 [details]
Patch

[Luke Macpherson]

Created attachment 110960 [details]
Patch

[WebKit Review Bot]

Attachment 110960 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Last 3072 characters of output:
endering/style/RenderStyle.h:206:  _white_space is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:207:  _box_direction is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:213:  _pointerEvents is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:214:  _insideLink is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:250:  _effectiveDisplay is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:251:  _originalDisplay is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:252:  _overflowX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:253:  _overflowY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:254:  _vertical_align is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:255:  _clear is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:256:  _position is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:257:  _floating is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:258:  _table_layout is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:260:  _page_break_before is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:261:  _page_break_after is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:262:  _page_break_inside is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:264:  _styleType is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:268:  _pseudoBits is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:269:  _unicodeBidi is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 29 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Luke Macpherson]

So, interestingly, I think this picked up a real bug on the windows build. These structs were much larger on windows (I don't remember the exact size). rearranging the order of the fields on windows got it down to ~13 bytes, and changing them to unsigned char (instead of unsigned) got them down to the same size as on mac.

The style errors here are existing naming errors where I have simply changed the type from unsigned to unsigned char.

[Darin Adler]

Comment on attachment 110960 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110960&action=review

These assertions should be in the .cpp file so they are only compiled once. And if you have some need to change these all to “unsigned char”, then please explain why.

> Source/WebCore/rendering/style/RenderStyle.h:172
> +            COMPILE_ASSERT((sizeof(InheritedFlags) <= 8), InheritedFlags_does_not_grow);

I’m not sure I understand why this is in the == operator, and further, why it is in the header file, not the .cpp file.

> Source/WebCore/rendering/style/RenderStyle.h:218
> +        unsigned char _empty_cells : 1; // EEmptyCell
> +        unsigned char _caption_side : 2; // ECaptionSide
> +        unsigned char _list_style_type : 7; // EListStyleType
> +        unsigned char _list_style_position : 1; // EListStylePosition
> +        unsigned char _visibility : 2; // EVisibility
> +        unsigned char _text_align : 4; // ETextAlign
> +        unsigned char _text_transform : 2; // ETextTransform
> +        unsigned char _text_decorations : ETextDecorationBits;
> +        unsigned char _cursor_style : 6; // ECursor
> +        unsigned char _direction : 1; // TextDirection
> +        unsigned char _border_collapse : 1; // EBorderCollapse
> +        unsigned char _white_space : 3; // EWhiteSpace
> +        unsigned char _box_direction : 1; // EBoxDirection (CSS3 box_direction property, flexible box layout module)
>          // 34 bits
>          
>          // non CSS2 inherited
> -        unsigned m_rtlOrdering : 1; // Order
> +        unsigned char m_rtlOrdering : 1; // Order
>          bool _force_backgrounds_to_white : 1;
> -        unsigned _pointerEvents : 4; // EPointerEvents
> -        unsigned _insideLink : 2; // EInsideLink
> +        unsigned char _pointerEvents : 4; // EPointerEvents
> +        unsigned char _insideLink : 2; // EInsideLink
>          // 43 bits
>  
>          // CSS Text Layout Module Level 3: Vertical writing support
> -        unsigned m_writingMode : 2; // WritingMode
> +        unsigned char m_writingMode : 2; // WritingMode

Why are you adding “char” to all of these? It seems unrelated to the assertion and not an improvement.

[Darin Adler]

OK, I see now, the change from "unsigned" to "unsigned char" is a Windows-specific fix or perhaps compiler bug workaround.

This needs to be mentioned in the change log.

[Luke Macpherson]

Created attachment 111207 [details]
Patch

[WebKit Review Bot]

Attachment 111207 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Last 3072 characters of output:
endering/style/RenderStyle.h:205:  _white_space is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:206:  _box_direction is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:212:  _pointerEvents is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:213:  _insideLink is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:248:  _effectiveDisplay is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:249:  _originalDisplay is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:250:  _overflowX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:251:  _overflowY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:252:  _vertical_align is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:253:  _clear is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:254:  _position is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:255:  _floating is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:256:  _table_layout is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:258:  _page_break_before is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:259:  _page_break_after is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:260:  _page_break_inside is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:262:  _styleType is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:266:  _pseudoBits is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:267:  _unicodeBidi is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 29 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 111207 [details]
Patch

Clearing flags on attachment: 111207

Committed r97712: <http://trac.webkit.org/changeset/97712>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.