Bug 97563 – GIFImageReader: fix tautological compare

• Component: New Bugs
• Status: RESOLVED FIXED

[Hans Wennborg]

GIFImageReader: fix tautological compare

[Hans Wennborg]

Created attachment 165601 [details]
Patch

[Hans Wennborg]

Adam: you had the pleasure of touching this file in Bug 52363, perhaps you can take a look?

[WebKit Review Bot]

Attachment 165601 [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
Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:659:  disposal_method is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 2 files


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

[Hans Wennborg]

(In reply to comment #3)
> Attachment 165601 [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
> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:659:  disposal_method is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> Total errors found: 1 in 2 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

The style is consistent with the rest of the file.

[Adam Barth]

Comment on attachment 165601 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        This patch fixes the code by doing the comparison before storing the
> +        value in the enum.

Is there any observable consequence of this change?  If so, should we write a test?

[Adam Barth]

That file is also massively out of style, but I agree that point-fixing this one issue isn't the right thing to do in this patch.

[Hans Wennborg]

(In reply to comment #5)
> (From update of attachment 165601 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165601&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        This patch fixes the code by doing the comparison before storing the
> > +        value in the enum.
> 
> Is there any observable consequence of this change?  If so, should we write a test?

There's no observable difference.

The code currently relied on unspecified behavior (i.e. that the enum can represent more values than it strictly has to), and my change just fixes that.

[Adam Barth]

Comment on attachment 165601 [details]
Patch

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

>>> Source/WebCore/ChangeLog:15
>>> +        value in the enum.
>> 
>> Is there any observable consequence of this change?  If so, should we write a test?
> 
> There's no observable difference.
> 
> The code currently relied on unspecified behavior (i.e. that the enum can represent more values than it strictly has to), and my change just fixes that.

Ah, I see.  We have storage for all the bits, they're just outside the enum range.  Makes sense.

[WebKit Review Bot]

Comment on attachment 165601 [details]
Patch

Clearing flags on attachment: 165601

Committed r129523: <http://trac.webkit.org/changeset/129523>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.