520672 - Fix warnings "warning: overflow in implicit constant conversion" in jpeg/jdmaster.c

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: warning log

When I'm building mozilla-central in the mozilla/jpeg/ directory, I get a ton of warning spam -- 512 consecutive copies of this warning:
> jdmaster.c:46: warning: overflow in implicit constant conversion
(ranging from lines 46 up to line 80, with multiple warnings per line)

These are because we're storing values from 0x80 up to 0xff (128 to 255) in an array of normal char values, whose range is restricted to -0x80 to 0x7F  (-128 to +127).

We should really be using an _unsigned_ char for the storage -- that gets rid of these warnings.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix

simple fix: add "unsigned" to the array declaration.

Comment 2

[Daniel Holbert [:dholbert]]

I've noticed that some other files in the jpeg directory act like we can't assume unsigned chars -- e.g. this chunk of http://mxr.mozilla.org/mozilla-central/source/jpeg/jmorecfg.h:

64 #ifdef HAVE_UNSIGNED_CHAR
65 
66 typedef unsigned char JSAMPLE;
67 #define GETJSAMPLE(value)  ((int) (value))
68 
69 #else /* not HAVE_UNSIGNED_CHAR */
70 
71 typedef char JSAMPLE;

I could use " #ifdef HAVE_UNSIGNED_CHAR" here as well, if desired... I'm not sure if there are any platforms we care about that don't have unsigned char, or if that's just a stale keyword.

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 404724 [details] [diff] [review]
fix

stuart's plate is pretty full and he's not in charge of imagelib anymore anyway. Reflagging joe.

Comment 4

[Daniel Holbert [:dholbert]]

Thanks bholley! I knew tagging stuart was probably a silly idea -- I just picked him 'cause I was in a hurry and I noticed he'd reviewed the last patch to touch this file. :)

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 404724 [details] [diff] [review]
fix

oh, I didn't actually look at the patch. Despite the fact that I'm a peer in libjpeg for name only, it's trivial enough for anyone to review.

r=bholley

Comment 6

[Bobby Holley (:bholley)]

make that r+ contingent on adding this to MOZCHANGES.

Comment 7

[Bobby Holley (:bholley)]

dholbert pointed out that the code he's changing was added in 2007 and MOZCHANGES hasn't been updated since 2003. So nevermind.

grumble grumble stuart grumble...

Comment 8

[Daniel Holbert [:dholbert]]

Also, regarding HAVE_UNSIGNED_CHAR - bholley pointed out that we use "unsigned char" elsewhere (e.g. PRUint8 & PRPackedBool are both typedefs for unsigned char in prtypes.h), so there's not really any point in pretending we accommodate platforms that don't support unsigned char in this particular spot.

Patch pushed:
http://hg.mozilla.org/mozilla-central/rev/dcd991f71c3f

Comment 9

[Daniel Holbert [:dholbert]]

Hrm, in retrospect, it'd probably be slightly cleaner to use "JSAMPLE" there instead of "char" (what it was before) *or* "unsigned char" (what this bug changed it to), because JSAMPLE is #typedef'd to unsigned char anyway, and the array size is computed based on sizeof(JSAMPLE).

Probably not a big deal though.  I can push another patch to do that if desired.

Comment 10

[Bobby Holley (:bholley)]

if the array size is computed based on JSAMPLE, please do push another patch using it. r=bholley on it.

Comment 11

[Daniel Holbert [:dholbert]]

pushed follow-up from comment 10:
http://hg.mozilla.org/mozilla-central/rev/17fbbaf79612