Closed Bug 653295 Opened 9 years ago Closed 9 years ago

nsFrameSetFrame.cpp:1391 warning: comparison between signed and unsigned integer expressions (or: Why are numCells/mNumRows/mNumCols signed?)

Categories

(Core :: Layout, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: dholbert, Assigned: atulagrwl)

References

(Blocks 1 open bug)

Details

(Whiteboard: [build_warning])

Attachments

(1 file, 1 obsolete file)

Filing bug on this build warning:
> nsFrameSetFrame.cpp:1391:27: warning: comparison between signed and unsigned integer expressions
for this code:

> 1351 void
> 1352 nsHTMLFramesetFrame::RecalculateBorderResize()
> 1353 {
[...]
> 1359   PRInt32 numCells = mNumRows * mNumCols; // max number of cells
[...]
> 1362   nsAutoArrayPtr<PRInt32> childTypes(new PRInt32[numCells]);
[...]
> 1366   PRUint32 childIndex, childTypeIndex = 0;
[...]
> 1387   for (; childTypeIndex < numCells; ++childTypeIndex) {

I think we can safely assume numCells is non-negative.  If it were negative, then "new PRInt32[numCells])" will convert it to a huge positive number and probably run out of memory.

So the most straightforward fix for this warning is probably to make numCells a PRUint32.  The only reason it's signed right now is presumably because mNumRows and mNumCols are signed... but I'm not sure why we'd want them to be signed, either.  If we somehow end up with a negative number of rows/cols, we're in trouble due to things like the "new" expression above overflowing.
(Based on nsHTMLFrameSetElement::ParseRowCol() which actually initialize the values of mNumRows / mNumCols, there's no way for them to end up with a negative value, at first glance...  So IMHO, we might want to make them unsigned too, and make ParseRowCol() etc. take an unsigned outparam)
Attached patch Patch v1 (obsolete) — Splinter Review
It is much easier to change the Unsigned to signed to avoid this warning. Change the signed to unsigned will require to change a lot of instances of variable declaration in the file. Moreover there is no danger of variable overflow as stated by the bug reporter as well. However I agree that making mNumRows and mNumCols is more appropriate solution but current patch might be best solution in this case (in order to make several changes in the file).
Assignee: nobody → atulagrwl
Attachment #558151 - Flags: review?(bzbarsky)
Comment on attachment 558151 [details] [diff] [review]
Patch v1

As long as you're touching this code, could you move it away from GetChildCount()/GetChildAt() to using GetFirstChild()/GetNextSibling()?
Attachment #558151 - Flags: review?(bzbarsky) → review-
Attached patch Patch v2Splinter Review
Improvements suggested by reviewer.
Attachment #558151 - Attachment is obsolete: true
Attachment #562323 - Flags: review?(bzbarsky)
Comment on attachment 562323 [details] [diff] [review]
Patch v2

r=me
Attachment #562323 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/7d983fc8a13c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 563226
You need to log in before you can comment on or make changes to this bug.