Closed Bug 1012814 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Widget, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.)
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 )
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8425008 - Flags: review?(bjacob)
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.
Attachment #8425031 - Flags: review?(bjacob)
er, I guess I accidentally qreffed in the "z" changes. New patch coming, without that. :)
Attachment #8425031 - Attachment description: par 2: switch nsTArray iteration to use size_t in nsTransferable.cpp → part 2, with additional s/i/z/, to demonstrate that these variables are only used in ElementAt() calls
(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 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.
Attachment #8425008 - Flags: review?(bjacob) → review+
Attachment #8425033 - Flags: review?(bjacob) → review+
https://hg.mozilla.org/mozilla-central/rev/9c12250ddf17
https://hg.mozilla.org/mozilla-central/rev/92982748e41b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.