Bug 82793

Summary: Add a compile assert for the size of BidiContext
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: New BugsAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eric, krit, leviw, mitz
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch eric: review+

Ryosuke Niwa
Reported 2012-03-30 15:42:59 PDT
Add a compile assert for the size of BidiContext
Attachments
Patch (2.28 KB, patch)
2012-03-30 15:47 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Patch (3.07 KB, patch)
2012-03-30 16:11 PDT, Ryosuke Niwa 		eric: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2012-03-30 15:47:22 PDT
Created attachment 134903 [details] Patch
Ryosuke Niwa
Comment 2 2012-03-30 15:48:13 PDT
Unfortunately, very few people can review changes to this file :(
Eric Seidel (no email)
Comment 3 2012-03-30 15:55:26 PDT
Comment on attachment 134903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134903&action=review > Source/WebCore/platform/text/BidiContext.h:62 > - unsigned char m_level; > + unsigned m_level : 6; You should add a comment which explains that we only need to handle 64 bidi-levels. Do we have tests for overflow of this value?
Ryosuke Niwa
Comment 4 2012-03-30 15:56:30 PDT
(In reply to comment #3) > (From update of attachment 134903 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134903&action=review > > > Source/WebCore/platform/text/BidiContext.h:62 > > - unsigned char m_level; > > + unsigned m_level : 6; > > You should add a comment which explains that we only need to handle 64 bidi-levels. Do we have tests for overflow of this value? Maybe we should define numBidiLevelBits?
Ryosuke Niwa
Comment 5 2012-03-30 16:11:34 PDT
Created attachment 134910 [details] Patch
Eric Seidel (no email)
Comment 6 2012-03-30 16:17:44 PDT
Comment on attachment 134910 [details] Patch OK. LGTM.
Ryosuke Niwa
Comment 7 2012-03-30 16:27:56 PDT
Committed r112742: <http://trac.webkit.org/changeset/112742>
Darin Adler
Comment 8 2012-03-30 18:25:18 PDT
Comment on attachment 134910 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134910&action=review > Source/WebCore/platform/text/BidiContext.cpp:31 > + uint32_t bitfields : 16; Tiny style question. Why use uint32_t here instead of unsigned?
Ryosuke Niwa
Comment 9 2012-03-30 21:43:49 PDT
(In reply to comment #8) > (From update of attachment 134910 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134910&action=review > > > Source/WebCore/platform/text/BidiContext.cpp:31 > > + uint32_t bitfields : 16; > > Tiny style question. Why use uint32_t here instead of unsigned? I was just following the pattern used elsewhere. We can change all of them to use unsigned instead.
Note You need to log in before you can comment on or make changes to this bug.