Clean up compiler warnings in nsCellMap.cpp

RESOLVED FIXED

Status

()

defect
P2
normal
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: caillon, Assigned: bernd_mozilla)

Tracking

(Blocks 1 bug)

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [whitebox])

Attachments

(3 attachments)

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.
Posted patch PatchSplinter Review
/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).
Please be sure to run the regression tests (see 
layout/doc/regression_tests.html).
Priority: -- → P2
Comment on attachment 98559 [details] [diff] [review]
Patch

the patch looks good to me , further the patch passes the layout regression
tests
Attachment #98559 - Flags: review+
Attachment #98559 - Flags: superreview?(dbaron)
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).
Attachment #98559 - Flags: superreview?(dbaron) → superreview-
Whiteboard: [whitebox]
Blocks: buildwarning
Posted 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]
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.
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]
patch

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