Summary: | Add a compile assert for the size of RenderBlock | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||
Component: | Layout and Rendering | Assignee: | Ryosuke Niwa <rniwa> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | darin, eric, kling, koivisto, morrita, ojan, tkent, tony, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Ryosuke Niwa
2012-03-29 02:05:18 PDT
Created attachment 134531 [details]
Initial attempt
For FloatingObject and MarginInfo, we could either define asserts in some methods of RenderBlock or add public methods that return the size of those classes. Comment on attachment 134531 [details] Initial attempt View in context: https://bugs.webkit.org/attachment.cgi?id=134531&action=review > Source/WebCore/ChangeLog:9 > + We can't add asserts for FloatingObject and MarginInfo because they're private to RenderBlock. You can put the COMPILE_ASSERT in the constructor. At least, that's what we do for RenderStyle::(Non)InheritedFlags. Comment on attachment 134531 [details] Initial attempt View in context: https://bugs.webkit.org/attachment.cgi?id=134531&action=review > Source/WebCore/rendering/RenderBlock.cpp:79 > +COMPILE_ASSERT(sizeof(RenderBlock) == sizeof(SameSizeAsRenderBlock), RenderBlock_should_stay_small); Can we use an absolute number instead? Comment on attachment 134531 [details] Initial attempt View in context: https://bugs.webkit.org/attachment.cgi?id=134531&action=review >> Source/WebCore/rendering/RenderBlock.cpp:79 >> +COMPILE_ASSERT(sizeof(RenderBlock) == sizeof(SameSizeAsRenderBlock), RenderBlock_should_stay_small); > > Can we use an absolute number instead? Pointers are different sizes on 32bit vs 64bit architectures. Comment on attachment 134531 [details] Initial attempt View in context: https://bugs.webkit.org/attachment.cgi?id=134531&action=review >>> Source/WebCore/rendering/RenderBlock.cpp:79 >>> +COMPILE_ASSERT(sizeof(RenderBlock) == sizeof(SameSizeAsRenderBlock), RenderBlock_should_stay_small); >> >> Can we use an absolute number instead? > > Pointers are different sizes on 32bit vs 64bit architectures. I see. I'm more worried that the bitfields will be padded differently, but I guess that's tested by other COMPILE_ASSERTS anyway. Comment on attachment 134531 [details] Initial attempt View in context: https://bugs.webkit.org/attachment.cgi?id=134531&action=review > Source/WebCore/rendering/RenderBlock.cpp:76 > + uint32_t m_bitfields : 32; Do you need the : 32 here? Comment on attachment 134531 [details] Initial attempt View in context: https://bugs.webkit.org/attachment.cgi?id=134531&action=review >> Source/WebCore/rendering/RenderBlock.cpp:76 >> + uint32_t m_bitfields : 32; > > Do you need the : 32 here? Oh yeah, that's redundant. Will fix. Created attachment 134631 [details]
Patch for landing
Comment on attachment 134631 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=134631&action=review > Source/WebCore/rendering/RenderBlock.cpp:85 > + uint32_t bitfields : 8; I'm not a huge fan of using bitfields in these SameSize* structs. One of the major benefits of these compile asserts is that they ensure we get the same size we expect with visual studio, which we know does weird things with bitfields. Maybe make this uint8_t? > Source/WebCore/rendering/RenderBlock.cpp:91 > + uint32_t bitfields : 16; Ditto, use uint16_t? (In reply to comment #10) > (From update of attachment 134631 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134631&action=review > > > Source/WebCore/rendering/RenderBlock.cpp:85 > > + uint32_t bitfields : 8; > > I'm not a huge fan of using bitfields in these SameSize* structs. One of the major benefits of these compile asserts is that they ensure we get the same size we expect with visual studio, which we know does weird things with bitfields. Maybe make this uint8_t? I don't think that'll work. Some compilers will always align sequence of bitfields to be one word (4/8 bytes) even though they may not pad other POD types smaller than int. Also, most of compilers pad struct so that the total size of struct is divisible by 4/8 unless the total size is smaller than 4/8 bytes. Comment on attachment 134631 [details] Patch for landing Clearing flags on attachment: 134631 Committed r112566: <http://trac.webkit.org/changeset/112566> All reviewed patches have been landed. Closing bug. |