1012814 - nsTransferable.cpp:467:57: warning: comparison is always true due to limited range of data type [-Wtype-limits]

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Daniel Holbert [:dholbert]]

New build warning, in my 64-bit build:
{
widget/xpwidgets/nsTransferable.cpp:467:57: warning: comparison is always true due to limited range of data type [-Wtype-limits]
   if ((idx = GetDataForFlavor(mDataArray, aDataFlavor)) != mDataArray.NoIndex) {
                                                         ^
}

This is because 'idx' here is a uint32_t, whereas nsTArray's index_type (and hence NoIndex) have now changed to be "size_t".

(Aside: This seems to be a case where bug 1004098 changed behavior. I'd expect any code that compares a uint32_t variable against NoIndex (e.g. to check the return value of IndexOf) will now be broken, in 64-bit builds.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1 (fixes the build warning)

This patch
 (1) makes us use size_t for this comparison
 (2) pulls the GetDataForFlavor function-call outside of the "if" check, to make the code more readable.

(Note: GetDataForFlavor() returns type size_t, as shown here:
http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsTransferable.cpp?rev=b347f6eb2239#40 )

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: part 2, with additional s/i/z/, to demonstrate that these variables are only used in ElementAt() calls

This switches us to use size_t in the nsTArray iteration loops in this file, while I'm here.

I verified[1] that these loop variables are *only* used for ElementAt() calls (and loop counting), so I don't think there should be any side-effects from this change.

[1] I verified this locally by adding a third patch that renames all these variables to "z" instead of "i", in their for(...) statements, and then replacing ElementAt(i) with ElementAt(z) globally in this file.  I was still able to compile, which means there shouldn't be any other usages.

Comment 3

[Daniel Holbert [:dholbert]]

er, I guess I accidentally qreffed in the "z" changes. New patch coming, without that. :)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: part 2: switch nsTArray iteration to use size_t in nsTransferable.cpp

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> (Aside: This seems to be a case where bug 1004098 changed behavior. I'd
> expect any code that compares a uint32_t variable against NoIndex (e.g. to
> check the return value of IndexOf) will now be broken, in 64-bit builds.)

(Happily, I think there are probably no other instances of this problem in mozilla-central. At least, none caught by gcc's "-Wtype-limits" build warning, according to ./mach warnings-list.)

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8425008 [details] [diff] [review]
part 1 (fixes the build warning)

Review of attachment 8425008 [details] [diff] [review]:
-----------------------------------------------------------------

Note that a IndexOf() API change was proposed on the dev-platform PSA thread that would catch such bugs at compile time. Not sure whether this occurrence is enough motivation to go down that route.

Comment 7

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c12250ddf17
https://hg.mozilla.org/integration/mozilla-inbound/rev/92982748e41b

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9c12250ddf17
https://hg.mozilla.org/mozilla-central/rev/92982748e41b