Closed
Bug 1012814
Opened 10 years ago
Closed 10 years ago
nsTransferable.cpp:467:57: warning: comparison is always true due to limited range of data type [-Wtype-limits]
Categories
(Core :: Widget, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: dholbert, Assigned: dholbert)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
1009 bytes,
patch
|
bjacob
:
review+
|
Details | Diff | Splinter Review |
4.34 KB,
patch
|
bjacob
:
review+
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Comment 1•10 years ago
|
||
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 | ||
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
er, I guess I accidentally qreffed in the "z" changes. New patch coming, without that. :)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8425031 -
Attachment is obsolete: true
Attachment #8425031 -
Flags: review?(bjacob)
Attachment #8425033 -
Flags: review?(bjacob)
Assignee | ||
Updated•10 years ago
|
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
Assignee | ||
Comment 5•10 years ago
|
||
(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•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8425033 -
Flags: review?(bjacob) → review+
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c12250ddf17 https://hg.mozilla.org/integration/mozilla-inbound/rev/92982748e41b
Flags: in-testsuite-
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9c12250ddf17 https://hg.mozilla.org/mozilla-central/rev/92982748e41b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•