Bug 72083 - Switch table indexing to unsigned
Summary: Switch table indexing to unsigned
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-10 18:35 PST by Julien Chaffraix
Modified: 2011-11-15 18:04 PST (History)
2 users (show)

See Also:


Attachments
Proposed change. (32.73 KB, patch)
2011-11-10 18:56 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Fixed the reverse iterators after Darin's suggestion. (37.65 KB, patch)
2011-11-14 16:36 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (37.67 KB, patch)
2011-11-15 17:47 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2011-11-10 18:35:29 PST
Follow up of bug 70369 where we switched only row / column handling to unsigned, now let's switch the remaining table code to unsigned (where it makes sense).
Comment 1 Julien Chaffraix 2011-11-10 18:56:30 PST
Created attachment 114614 [details]
Proposed change.
Comment 2 Darin Adler 2011-11-14 12:03:28 PST
Comment on attachment 114614 [details]
Proposed change.

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

> Source/WebCore/ChangeLog:18
> +        The only exception is reverse iterating over the array
> +        as I did not find a good way of doing that without using
> +        an unsigned (tried all of the usual suggestions but they
> +        keep failing locally and using do { } while () is worse).
> +        Due to the size limits on the columns and rows added in
> +        bug 71135, it should be difficult to overflow the integer
> +        in some cases.

for (unsigned i = size; i; ) {
        --i;
        // Body of loop goes here.
    }
Comment 3 Julien Chaffraix 2011-11-14 16:31:32 PST
(In reply to comment #2)
> (From update of attachment 114614 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114614&action=review
> 
> > Source/WebCore/ChangeLog:18
> > +        The only exception is reverse iterating over the array
> > +        as I did not find a good way of doing that without using
> > +        an unsigned (tried all of the usual suggestions but they
> > +        keep failing locally and using do { } while () is worse).
> > +        Due to the size limits on the columns and rows added in
> > +        bug 71135, it should be difficult to overflow the integer
> > +        in some cases.
> 
> for (unsigned i = size; i; ) {
>         --i;
>         // Body of loop goes here.
>     }

Exactly what I needed, thanks! I tried it and it works perfectly without being too horrible. Better patch forthcoming.
Comment 4 Julien Chaffraix 2011-11-14 16:36:23 PST
Created attachment 115056 [details]
Fixed the reverse iterators after Darin's suggestion.
Comment 5 Darin Adler 2011-11-15 11:46:07 PST
Comment on attachment 115056 [details]
Fixed the reverse iterators after Darin's suggestion.

4>c:\cygwin\home\buildbot\webkit\source\webcore\rendering\RenderTableSection.cpp(1206) : warning C4804: '>=' : unsafe use of type 'bool' in operation

Probably need a ? 1 : 0 in there to make Windows build.
Comment 6 Julien Chaffraix 2011-11-15 17:47:04 PST
Created attachment 115289 [details]
Patch for landing
Comment 7 WebKit Review Bot 2011-11-15 18:04:14 PST
Comment on attachment 115289 [details]
Patch for landing

Clearing flags on attachment: 115289

Committed r100386: <http://trac.webkit.org/changeset/100386>
Comment 8 WebKit Review Bot 2011-11-15 18:04:19 PST
All reviewed patches have been landed.  Closing bug.