Closed Bug 167659 Opened 18 years ago Closed 18 years ago

Clean up compiler warnings in nsCellMap.cpp


(Core :: Layout: Tables, defect, P2)






(Reporter: caillon, Assigned: bernd_mozilla)


(Blocks 1 open bug)


(Whiteboard: [whitebox])


(3 files)

Warnings usually look harmless, but these were good ones.  I was able to remove
some unneeded code, clarify and slightly improve perf of some code, and fix some
real issues.  Patch coming up.
Attached patch PatchSplinter Review
/usr/local/src/mozilla/layout/html/table/src/nsCellMap.cpp: In method
/usr/local/src/mozilla/layout/html/table/src/nsCellMap.cpp:108: warning: unused
variable `PRInt32 rowCount'

Simply removed the unused variable.

/usr/local/src/mozilla/layout/html/table/src/nsCellMap.cpp: In method `class
BCData * nsTableCellMap::GetRightMostBorder(int)':
/usr/local/src/mozilla/layout/html/table/src/nsCellMap.cpp:136: warning: `class
BCData * bcData' might be used uninitialized in this function

The compiler is kind of stupid here.  bcData will always be initialized, but it
is kind of hard to see at first glance.  This can be re-written for both
efficiency and to make it plainer to the compiler that we know we'll have a

/usr/local/src/mozilla/layout/html/table/src/nsCellMap.cpp: In method `class
BCData * nsTableCellMap::GetBottomMostBorder(int)':
/usr/local/src/mozilla/layout/html/table/src/nsCellMap.cpp:157: warning: `class
BCData * bcData' might be used uninitialized in this function

Same as above.

/usr/local/src/mozilla/layout/html/table/src/nsCellMap.cpp: In method `class
BCData * nsTableCellMap::GetBCData(unsigned char, nsCellMap &, unsigned int,
unsigned int, int = 0)':
/usr/local/src/mozilla/layout/html/table/src/nsCellMap.cpp:767: warning: unused
variable `class BCData * bcData'

This looks wrong.  We don't want to declare a local bcData which loses scope
right away, we just want to assign to the existing one.

/usr/local/src/mozilla/layout/html/table/src/nsCellMap.cpp:781: warning: unused
variable `class BCData * bcData'

Same here.

/usr/local/src/mozilla/layout/html/table/src/nsCellMap.cpp: In method `void
nsCellMap::ExpandWithCells(nsTableCellMap &, nsVoidArray &, int, int, int, int,
nsRect &)':
/usr/local/src/mozilla/layout/html/table/src/nsCellMap.cpp:1474: warning:
`PRInt32 endColIndex' might be used uninitialized in this function

Technically, there is one case here where we can not be initialized: and that
is |if (aCellFrames.Count() == 0)| but I'm not sure that will ever happen. 
Nonetheless it is possible in theory, and since we initialized totalColSpan to
0, it appears we should do the same for endColIndex.

On top of that, while I was trying to understand this code, I noticed some
logic that could be cleaned up:

  PRInt32 startColIndex = aColIndex;
  for (PRInt32 cellX = 0; cellX < numCells; cellX++) {
    if (cellX == 0) {
      endColIndex = aColIndex + colSpan - 1;
    else {
      startColIndex = endColIndex + 1;
      endColIndex   = startColIndex + colSpan - 1;

Since we know on the first iteration that we haven't touched startColIndex yet,
and it is equal to aColIndex, we can use startColIndex instead.  After that, we
can easily condense the if/else into just one if (cellX > 0).
Please be sure to run the regression tests (see 
Priority: -- → P2
Comment on attachment 98559 [details] [diff] [review]

the patch looks good to me , further the patch passes the layout regression
Attachment #98559 - Flags: review+
Attachment #98559 - Flags: superreview?(dbaron)
Comment on attachment 98559 [details] [diff] [review]

>-          BCData* bcData = GetBottomMostBorder(aColIndex);
>+          bcData = GetBottomMostBorder(aColIndex);

>-      BCData* bcData = GetRightMostBorder(aRowIndex);
>+      bcData = GetRightMostBorder(aRowIndex);

Have you figured out what these changes do?  If you can't find any tests where
they change the behavior, why do we want this code in the tree?

>-    if (cellX == 0) {
>-      endColIndex = aColIndex + colSpan - 1;
>-    }
>-    else {
>+    if (cellX > 0) {
>       startColIndex = endColIndex + 1;
>-      endColIndex   = startColIndex + colSpan - 1;
>     }
>+    endColIndex = startColIndex + colSpan - 1;

I'm not convinced this is equivalent to the old code -- I'll look more closely
in a bit.

I also wonder if it might be clearer to write the loops that you rewrote as
just a for loop followed by an array access rather than trying to optimize away
a single array access in the case where you're allocating (which is quite slow
anyway, since it's not arena allocation).
Attachment #98559 - Flags: superreview?(dbaron) → superreview-
Whiteboard: [whitebox]
Blocks: buildwarning
Attached patch patchSplinter Review
I removed the the disputed part of the patch and changed the aRowIndex as an
argument to a 0 because the first row in the next cellmap will have certainly
the index 0 and not aRowIndex.
Attachment #112622 - Flags: review?(jkeiser)
Comment on attachment 112622 [details] [diff] [review]

r=jkeiser, but since the do { } while() isn't buying you anything except one
less <= check, please change it back to a for(), which is more readable and
Attachment #112622 - Flags: review?(jkeiser) → review+
I lost track of this one
Assignee: caillon → bernd_mozilla
I would prefer to keep the do{} while(), it makes sure that bcdata is really
initialized once and silences the compiler warning.
Comment on attachment 112622 [details] [diff] [review]

Fair enough, go ahead.
Attachment #112622 - Flags: superreview?(dbaron)
Attachment #112622 - Flags: superreview?(dbaron) → superreview+
fix checked in
marking fixed
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.