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

RESOLVED FIXED in mozilla9

Status

()

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: dholbert, Assigned: atulagrwl)

Tracking

(Blocks 1 bug)

Trunk
mozilla9
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [build_warning])

Attachments

(1 attachment, 1 obsolete attachment)

1.53 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
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)
Posted 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-
Posted 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: 8 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.