Last Comment Bug 653295 - nsFrameSetFrame.cpp:1391 warning: comparison between signed and unsigned integer expressions (or: Why are numCells/mNumRows/mNumCols signed?)
: nsFrameSetFrame.cpp:1391 warning: comparison between signed and unsigned inte...
Status: RESOLVED FIXED
[build_warning]
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla9
Assigned To: Atul Aggarwal
:
Mentors:
: 563226 (view as bug list)
Depends on:
Blocks: buildwarning
  Show dependency treegraph
 
Reported: 2011-04-27 16:52 PDT by Daniel Holbert [:dholbert]
Modified: 2013-03-02 01:02 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (1.38 KB, patch)
2011-09-04 08:29 PDT, Atul Aggarwal
bzbarsky: review-
Details | Diff | Review
Patch v2 (1.53 KB, patch)
2011-09-25 13:21 PDT, Atul Aggarwal
bzbarsky: review+
Details | Diff | Review

Description Daniel Holbert [:dholbert] 2011-04-27 16:52:38 PDT
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.
Comment 1 Daniel Holbert [:dholbert] 2011-04-27 16:56:19 PDT
(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)
Comment 2 Atul Aggarwal 2011-09-04 08:29:21 PDT
Created attachment 558151 [details] [diff] [review]
Patch v1

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).
Comment 3 Boris Zbarsky [:bz] 2011-09-06 20:07:13 PDT
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()?
Comment 4 Atul Aggarwal 2011-09-25 13:21:54 PDT
Created attachment 562323 [details] [diff] [review]
Patch v2

Improvements suggested by reviewer.
Comment 5 Boris Zbarsky [:bz] 2011-09-25 21:00:46 PDT
Comment on attachment 562323 [details] [diff] [review]
Patch v2

r=me
Comment 7 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-26 07:46:29 PDT
https://hg.mozilla.org/mozilla-central/rev/7d983fc8a13c
Comment 8 :Cykesiopka 2013-03-02 01:02:40 PST
*** Bug 563226 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.