908514 - Fix warnings in nsICODecoder.cpp

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

nsICODecoder.cpp always generates warnings when compiling imagelib, which causes unnecessary debugging friction. There are two issues:

- Self-assignment warnings, caused by macros in EndianMacros.h.
- Cast-alignment warnings, which can be avoided through the use of a temporary variable.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Fix warnings in nsICODecoder.cpp.

Proposed patch. Try job here:

https://tbpl.mozilla.org/?tree=Try&rev=26cf8f00e965

Comment 2

[Seth Fowler [:seth] [:s2h]]

Actually, I changed my mind about the approach to fixing the self-assignment warnings. Let's just get rid of those macros altogether and replace them with template functions.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Replace imagelib endian macros with template functions.

Proposed patch. This renames EndianMacros.h to Endian.h and replaces all of the macros with template functions. The bodies of the template functions are taken directly from the original macros.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Fix cast alignment warning in nsICODecoder.

This is a tiny patch to fix a cast alignment warning in nsICODecoder.cpp.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Try job here:

https://tbpl.mozilla.org/?tree=Try&rev=d96a0c0b3299

Comment 6

[Seth Fowler [:seth] [:s2h]]

Looks like there is some bustage. Will look into this tomorrow.

Comment 7

[Masatoshi Kimura [:emk]]

Why don't you use mfbt/Endian.h?

Comment 8

[Jeff Muizelaar [:jrmuizel]]

(In reply to Masatoshi Kimura [:emk] from comment #7)
> Why don't you use mfbt/Endian.h?

I was going suggest the same.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Didn't know it existed. That's great. I'll switch it over.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Replace imagelib endian macros with MFBT's endian functions.

Redid the patch to use MFBT's Endian.h and fixed known orange tests.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Fix "variable may be used uninitialized" warning in nsICODecoder.cpp.

The new version of part 1 naturally takes care of the warning handled by the old part 2, but while verifying this I discovered that there's another warning produced by gcc in image/decoders. This new part 2 takes care of that warning by explicitly initializing a variable.

Comment 12

[Seth Fowler [:seth] [:s2h]]

New try job here: https://tbpl.mozilla.org/?tree=Try&rev=c911f1c44216

Comment 13

[Seth Fowler [:seth] [:s2h]]

Thanks for the review, Jeff!

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f846fad9b0f
https://hg.mozilla.org/integration/mozilla-inbound/rev/20287656b048

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1f846fad9b0f
https://hg.mozilla.org/mozilla-central/rev/20287656b048