52285 - Kill (unused & redeclaration) compiler warnings in layout/xul

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Jeffrey Baker]

Attached is a patch which fixes the legitimate compiler warnings in
mozilla/layout/xul/base/src.  Per the advice from bug 52234, I have not tried to
suppress any warnings that are due to GCC's lack of understanding.

The patch removes 7 completely unused variables and two useless redeclarations
of local variables.  CC those of you who were blamed in tinderbox (and brendan
because he is a smart and helpful guy).

Comment 1

[Jeffrey Baker]

Attachment: Patch

Comment 2

[Brendan Eich [:brendan]]

Most of these look like Eric's.

/be

Comment 3

[Jeffrey Baker]

In testing this patch breaks the UI layout.  I may have missed a side-effect
somewhere.  Stay tuned for a second, tested patch.

Comment 4

[Jeffrey Baker]

Slip of the keyboard in the nsSprocketLayout.cpp changes.  The second patch is
attached and smoketested on Linux.  Sorry for the noise.

Comment 5

[Jeffrey Baker]

Attachment: Corrected Patch

Comment 6

[Peter Trudelle]

Thanks for the patch, futuring to get off radar, but hopefully EVaughan 
will still have a chance to review this.

Comment 7

[Aleksey Nogin]

Is this bug completely forgotten? It has an almost two year old patch attached
to it...

Bug 132141 is also for warnings in layout/xul

Comment 8

[Aleksey Nogin]

Attachment: Clean up a bunch of warnings.

In a few cases I chose to throw away the unused rv wariable in a function that
always returns NS_OK. Would it be better to make it return rv instead?

Comment 9

[Aleksey Nogin]

CCing people blamed for some of the warnings my patch fixes.

Can somebody please review my patch? Thanks a lot!

Comment 10

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

Comment on attachment 84427 [details] [diff] [review]
Clean up a bunch of warnings.

>Index: layout/xul/base/src/nsBox.cpp
>-    sprintf(addr, "[@%p] ", frame);
>+    sprintf(addr, "[@%p] ", (void *) frame);

Prefer NS_STATIC_CAST(void*, frame) to old-style casts.

>Index: layout/xul/base/src/nsImageBoxFrame.cpp
>-  nsresult rv = nsLeafBoxFrame::AttributeChanged(aPresContext, aChild, aNameSpaceID, aAttribute, aModType, aHint);
>+  nsLeafBoxFrame::AttributeChanged(aPresContext, aChild, aNameSpaceID, aAttribute, aModType, aHint);

How about checking the return value and returning, i.e.,

if (NS_FAILED(rv))
  return rv;

>Index: layout/xul/base/src/nsMenuPopupFrame.cpp

Same here.

>Index: layout/xul/base/src/nsScrollBoxFrame.cpp

And here?

Then again, lots of layout code just ignores nsresult return
values, so they don't really mean much and it's probably not
that bad to ignore a few more...

Comment 11

[Aleksey Nogin]

Attachment: Clean up a bunch of warnings, v2

Incorporated David's suggestions - use NS_STATIC_CAST; do something reasonable
with previously unused rv's - in tow cases out of three I now return the rv
instead of discarding it, but in the third case just discarding it seems like a
natural thing to do.

Comment 12

[Aleksey Nogin]

Attachment: Clean up a bunch of warnings, v3 [Checked in: Comment 15]

Updating for bit rot.

Previous patch was sitting around with no activity for almost half a year.
Hopefully this one would be luckier.

Anybody care to review? Thanks!

Comment 13

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

Comment on attachment 105782 [details] [diff] [review]
Clean up a bunch of warnings, v3
[Checked in: Comment 15]

sr=dbaron

Comment 14

[Aleksey Nogin]

Can someboby please check this in (I do not have CVS access)? Thanks a lot!

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

this is now checked in
11/18/2002 21:06 timeless%mozdev.org mozilla/ layout/ xul/ base/ src/ grid/
nsGridRowLeafFrame.cpp 1.4 0/1  Bug 52285 Kill compiler warnings in layout/xul
patch by [e-mail] r=timeless sr=dbaron 
(and other files)

Comment 16

[Aleksey Nogin]

Comment on attachment 105782 [details] [diff] [review]
Clean up a bunch of warnings, v3
[Checked in: Comment 15]

This was checked in and fixed 12 or so warnings. 

There are still a number of meaningful warnings in layout/xul though, so we
should keep the bug open.

Comment 17

[Serge Gautherie (:sgautherie)]

Updating:
*(S) New -> R.Fixed, in an attempt to sort the many "warning" bugs.