206864 - Incorrect comparison in content/html/style/src/nsCSSParser.cpp

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[tenthumbs]

Gcc says: 

nsCSSParser.cpp:2780: warning: comparison of unsigned expression < 0 is always false

and the code is:

    2773   PRUint32 value = (PRUint32)NSToIntRound(mToken.mNumber*255);
    2774 
    2775   if (!ExpectSymbol(aErrorCode, ')', PR_TRUE)) {
    2776     REPORT_UNEXPECTED_TOKEN(NS_LITERAL_STRING("Expected ')' but found"));
    2777     return PR_FALSE;
    2778   }
    2779   
    2780   if (value < 0) value = 0;
    2781   if (value > 255) value = 255;
    2782   aOpacity = (PRUint8)value;

AS it stands a negative number becomes 255 not 0. Assuming you want to catch
negative values, then "value" must be signed. I suggest making it just an int.
The native type is, presumably, the most efficient.

Assigning it to bz because he did it in bug 160550.

Comment 1

[tenthumbs]

Attachment: simple patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

How about PRInt32 instead of int?

Comment 3

[tenthumbs]

What about a 64-bit platform? Using 32-bit quantities may incur 
additional overhead. It all depends on the instruction set. I generally 
prefer to let the compiler writers deal with this sort of stuff.

Comment 4

[Boris Zbarsky [:bzbarsky]]

See http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsUnitConversion.h#90 --
we're already converting to PRInt32.

Comment 5

[tenthumbs]

I wouldn't call nsUnitConversion.h a shining model of good coding 
practice. See bug 118117 for one way it's broken. It was also created 
essentially at the dawn of time (mozilla-wise). The code might well be 
derived from an earlier source. Since it uses IEEE floats which have 
just 24 bits of precision, perhaps PRInt32 is just to force something 
larger than that. Since mozilla always runs on at least a 32-bit 
platform forcing a special type seems useless to me.

However, I realize I'm probably alone in my views so I'll just hold my 
nose while you use PRInt32. The important thing is to use a signed 
quantity.

Comment 6

[Boris Zbarsky [:bzbarsky]]

It doesn't matter what you or I think of nsUnitConversion -- you're calling that
function, and it returns a PRInt32.  So in this case, on a 64-bit platform, it's
assigning to an 'int' that may have additional overhead.

And on platforms where an 'int' is less than 32 bits, you could overflow an
'int' if you assigned out of a PRInt32 into it.  So let's not do that, ok?  ;)

In any case, if you could post an updated patch that would be great, because I
have no tree to diff against.

Comment 7

[tenthumbs]

Well, mozilla isn't supposed to run on a 16-bit platform so I don't think that's
relevant.

I have to rant a little. I see pepple running around becoming upset because of 
millisecond run-time changes. I see other people insisting on specifying object
types (e.g. 8 and 16 bit integers) that have performance penalties. Yet, the
complainers never mention the obvious.

Anyway, stand by for the patch.

Comment 8

[tenthumbs]

Attachment: PRInt32 patch

Comment 9

[tenthumbs]

Attachment: the right patch, this time

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 124210 [details] [diff] [review]
the right patch, this time

r+sr=bzbarsky; please find someone to check this in?  (eg biesi or timeless,
maybe?)

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

ok, done:
Checking in mozilla/content/html/style/src/nsCSSParser.cpp;
/cvsroot/mozilla/content/html/style/src/nsCSSParser.cpp,v  <--  nsCSSParser.cpp
new revision: 3.223; previous revision: 3.222
done

Comment 12

[Boris Zbarsky [:bzbarsky]]

*** Bug 223391 has been marked as a duplicate of this bug. ***