167659 - Clean up compiler warnings in nsCellMap.cpp

Status: RESOLVED FIXED

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

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

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.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch

/usr/local/src/mozilla/layout/html/table/src/nsCellMap.cpp: In method
`nsTableCellMap::~nsTableCellMap()':
/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
bcData.


/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).

Comment 2

[karnaze (gone)]

Please be sure to run the regression tests (see 
layout/doc/regression_tests.html).

Comment 3

[Bernd]

Comment on attachment 98559 [details] [diff] [review]
Patch

the patch looks good to me , further the patch passes the layout regression
tests

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 98559 [details] [diff] [review]
Patch

>-          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).

Comment 5

[Bernd]

Attachment: testcase for the bcdata case as required by david

Comment 6

[Bernd]

Attachment: patch

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.

Comment 7

[John Keiser (jkeiser)]

Comment on attachment 112622 [details] [diff] [review]
patch

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
shorter.

Comment 8

[Bernd]

I lost track of this one

Comment 9

[Bernd]

I would prefer to keep the do{} while(), it makes sure that bcdata is really
initialized once and silences the compiler warning.

Comment 10

[John Keiser (jkeiser)]

Comment on attachment 112622 [details] [diff] [review]
patch

Fair enough, go ahead.

Comment 11

[Bernd]

fix checked in

Comment 12

[Bernd]

marking fixed

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

Thanks Bernd!