70989 - Clean up lots of "shadows" and other common warnings

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Comment 1

[timeless]

Attachment: patch 1

Comment 2

[Dan Rosen]

Heya, just cleaning up a bit here. Just wanted to suggest that you split the
patch up into one patch per top-level directory. That way you can grow each
chunk of the patch and have them still be reasonably easy to digest and review well.

Setting to Browser-General, since this isn't really Tracking, and setting myself
to QA, since I'm taking care of 70386.

Comment 3

[timeless]

Attachment: mozilla/content

Comment 4

[timeless]

Attachment: mozilla/dom/

Comment 5

[timeless]

Attachment: mozilla/uriloader/

Comment 6

[timeless]

Attachment: mozilla/view/

Comment 7

[timeless]

Attachment: mozilla/gfx/

Comment 8

[timeless]

Attachment: mozilla/xpcom/

Comment 9

[timeless]

Attachment: mozilla/modules/oji

Comment 10

[timeless]

Attachment: mozilla/embedding/

Comment 11

[timeless]

Attachment: mozilla/webshell/

Comment 12

[timeless]

Attachment: mozilla/layout/xul/

Comment 13

[timeless]

Attachment: mozilla/layout/html/

Comment 14

[timeless]

Attachment: mozilla/layout/svg/

Comment 15

[timeless]

Attachment: mozilla/layout/base/

Comment 16

[timeless]

Attachment: mozilla/extensions/cookie/

Comment 17

[timeless]

Attachment: mozilla/mailnews/base/

Comment 18

[timeless]

Attachment: mozilla/mailnews/db/

Comment 19

[timeless]

Attachment: mozilla/mailnews/imap/

Comment 20

[timeless]

Attachment: mozilla/mailnews/local/

Comment 21

[timeless]

Attachment: mozilla/mailnews/addrbook/

Comment 22

[timeless]

Attachment: mozilla/mailnews/mime/

Comment 23

[Peter Van der Beken [:peterv]]

r=peterv for http://bugzilla.mozilla.org/showattachment.cgi?attach_id=27044
 (mozilla/content)

Comment 24

[Johnny Stenback (:jst)]

The patch for mozilla/dom includes changes only in generated files so please
don't check that in.

I had a look at the changes in mozilla/content and I agree with everything but
this one change, in nsHTMLInputElement.cpp:

@@ -599,16 +599,12 @@
 
     nsIStatefulFrame::StateType stateType;
 
-    if (type == NS_FORM_INPUT_CHECKBOX) {
-      stateType = nsIStatefulFrame::eCheckboxType;
-    } else {
-      stateType = nsIStatefulFrame::eRadioType;
-    }
+    stateType = (type == NS_FORM_INPUT_CHECKBOX) 
+      ? nsIStatefulFrame::eCheckboxType : nsIStatefulFrame::eRadioType;

This change doesn't help in any way, all it does is makes the code harder to
read. If you revert the above change then sr=jst for the mozilla/content patch.

Comment 25

[Peter Van der Beken [:peterv]]

r=peterv for
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=27046 (mozilla/uriloader)
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=27047 (mozilla/view)
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=27048 (mozilla/gfx)
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=27051 (mozilla/embedding)
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=27052 (mozilla/webshell)

Comment 26

[Dan Rosen]

veto=dr on mozilla/mailnews/mime patch. that code is generated by lex -- don't
mess with it. congrats on your other reviews thus far, though -- great to see
this cleaning happening!

Comment 27

[Johnny Stenback (:jst)]

r=jst for the mozilla/layout/xul changes.

As for mozilla/layout/html:

Index: html/base/src/nsContainerFrame.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/base/src/nsContainerFrame.cpp,v
retrieving revision 1.101
diff -u -r1.101 nsContainerFrame.cpp
--- nsContainerFrame.cpp	2001/02/07 09:48:49	1.101
+++ nsContainerFrame.cpp	2001/03/07 22:35:24
@@ -146,8 +146,7 @@
                                 const nsRect& aDirtyRect,
                                 nsFramePaintLayer    aWhichLayer)
 {
-  const nsStyleDisplay* disp = (const nsStyleDisplay*)
-    mStyleContext->GetStyleData(eStyleStruct_Display);
+  mStyleContext->GetStyleData(eStyleStruct_Display);

   ^-- No need to call GetStyleData() here if disp wasn't used anywhere...

with that change, r=jst for mozilla/layout/html/, and r=jst for
mozilla/layout/base/ and mozilla/xpcom too.

Comment 28

[timeless]

Attachment: mozilla/extensions/cookie/ (updated)

Comment 29

[timeless]

Attachment: mozilla/content/ (patch per jst)

Comment 30

[timeless]

Attachment: mozilla/layout/html/ (patch per jst)

Comment 31

[Marc Attinasi]

layout/html: Why the #if 0 in the nsTableFrame.cpp and nsTableRowFrame.cpp? Can 
it just be removed? Also, did RODS request the #DEBUG_RODS blocks? If not, I'd 
just remove that commented-out old stuff because this is more confusing. And, 
what jst said about nsContainerFrame.cpp. [s]r=attinasi

layout/svg: [s]r=attinasi

layout/base: [s]r=attinasi

(what is the patch  
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=27861 
all about?)

Comment 32

[Jean-Francois Ducarroz]

mozilla/mailnews/base/
R=ducarroz

mozilla/mailnews/db/
R=ducarroz

mozilla/mailnews/imap/   
in nsImapIncomingServer.cpp, the first declaration of uri at line 477 is not
used therefore remove this line instead of fixing the second declaration (line 515)

mozilla/mailnews/local/
R=ducarroz

mozilla/mailnews/addrbook/
in nsAbDirectory.cpp, line 494, you cannot reuse the same variable i, you must
instead rename it j and change the second loop to use it else you will break the
logic!

mozilla/mailnews/mime/
R=ducarroz

Comment 33

[timeless]

Attachment: mozilla/mailnews/addrbook/ (patch per ducarroz)

Comment 34

[timeless]

Attachment: mozilla/mailnews/imap/ (patch per ducarroz)

Comment 35

[Jean-Francois Ducarroz]

R=ducarroz for all mailnews patches. Thanks for doing this clean up!

Comment 36

[Stephen P. Morse]

Regarding the fix for nsCookie.cpp (attachment id=27859), please hold off on 
doing this.  I am in the process of doing a major restructuring of the cookie 
module (see bug 46783) which involves adding new files, renaming the existing 
ones, and redistributing the contents of each file.  So making any fix at this 
time to the current set of files will be futile.

I do agree that the change indicated in attachment id=27859 is the right thing 
to do and I will incorporate it into the appropriate place in the new world 
order for cookies.  So this will be fixed by the checkin that I make for bug 
27859.

Comment 37

[Stephen P. Morse]

Oops, I meant to say bug 46783 rather than 27859 above.  The latter is your 
attachment number.

Comment 38

[edburns]

r=edburns for modules/oji

Comment 39

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

In view/src/nsView.cpp:
 * please use |#if 0| rather than |#ifdef 0|.
 * DEBUG_warren is inappropriate here.  Those lines were only attributed to
   warren because of the changes for and the backout of 47207.

In xpcom/tests/TestThreads.cpp
 * you probably want to change to %p rather than cast to PRUint32 (which is
   bad for platforms with 64-bit pointers).

Comment 40

[timeless]

Attachment: mozilla/view/ (patch per dbaron)

Comment 41

[timeless]

Attachment: mozilla/xpcom/ (patch per dbaron)

Comment 42

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

sr comments:
 * you need to cast to (void *) explicity to use it with %p on gcc these days.
   It's sucky, but it's true.

 * in webshell, you do:
   +      url = (nsString*) mPendingURLs.ElementAt(0);
   Do we want an NS_REINTERPRET_CAST here?

 * in the xul stuff, you have:
   +      if (parentBox)
   +         return parentBox->RelayoutDirtyChild(aState, this);
          else {
            nsIFrame* parent = nsnull;
   Wanna kill the heinous else-after-return there as well?

 * svg:
   +    pnt = (nsPoint*)mPoints.ElementAt(cnt);
   More NS_REINTERPRET_CAST here?

 * mailnews/base:
   +        msg = GetString(NS_LITERAL_STRING("PrintingMessage").GetUnicode());
   Is that safe?  What's the lifetime of the pointer returned by GetUnicode 
   there?

 * mailnews/addrbook:
   - nsCOMPtr<nsISupports> pSupport =    
         getter_AddRefs(pAddressLists->ElementAt(j));
   + pSupport = getter_AddRefs(pAddressLists->ElementAt(j));
     nsCOMPtr<nsIAbCard> cardInList(do_QueryInterface(pSupport, &rv));
   You have tabs here in your + line, and can we use QueryElementAt to save the
   explicit QI?

   - nsresult rv = NS_OK;
   + rv = NS_OK;
     NS_WITH_SERVICE(nsIPref, pPref, kPrefCID, &rv);
   More tabs, and why initialize it there when you're going to set it right 
   after in NS_WITH_SERVICE anyway?

Other than that, I like it a lot.  Tidy those up, and sr=shaver.

Comment 43

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

shaver wrote:
> * you need to cast to (void *) explicity to use it with %p on gcc these days.
>   It's sucky, but it's true.

Those warnings that were in gcc 2.96-RH seem to have gone away in gcc3.


One thing that bothers me in general about many of these changes is that they
use the same variable for 2 different things.  It seems it would be clearer to
rename the shadowed variable rather than reuse the old one by removing the new
second declaration.  In many cases (although not all) the compiler ought to
optimize away the difference, which I'd think is a drop in the bucket anyway for
both performance and stack space.  If others disagree with me, I don't care that
much, but I'd rather not make changes that could cause confusion later.

Comment 44

[timeless]

Attachment: mozilla/xpcom/ (revised patch per shaver)

Comment 45

[timeless]

Attachment: mozilla/webshell (patch per shaver)

Comment 46

[timeless]

Attachment: mozilla/mailnews/addrbook (revised patch per shaver)

Comment 47

[timeless]

Attachment: mozilla/mailnews/base (patch per shaver)

Comment 48

[timeless]

Attachment: mozilla/layout/xul (patch per shaver)

Comment 49

[timeless]

Attachment: mozilla/layout/svg (patch per shaver)

Comment 50

[timeless]

Attachment: collected+corrected patch

Comment 51

[timeless]

Ok, patch correctly uses NS_STATIC_CAST(const thingy*, item).

fixed one fatal thought/typ-o. patch compiles and runs w/o regression on linux 
:)

i'm sticking w/ the (void*) cast in a debug printf %p because there's no harm.

before shaver looked at this bug, i had:
r=peterv, sr=jst* attachment (id=27044) mozilla/content/
r=jst*, attachment (id=27054) mozilla/layout/html/
r=peterv, attachment (id=27046) mozilla/uriloader/
r=peterv, attachment (id=27047) mozilla/view/
r=peterv, attachment (id=27048) mozilla/gfx/
r=jst, attachment (id=27049) mozilla/xpcom/
r=peterv, attachment (id=27051) mozilla/embedding/
r=peterv, attachment (id=27052) mozilla/webshell/
r=jst, attachment (id=27053) mozilla/layout/xul/
r=jst, attachment (id=27056) mozilla/layout/base/
r=ducarroz, attachment (id=27059) mozilla/mailnews/base/
r=ducarroz, attachment (id=27060) mozilla/mailnews/db/
r=ducarroz*, attachment (id=27061) mozilla/mailnews/imap/
r=ducarroz, attachment (id=27062) mozilla/mailnews/local/
r=ducarroz*, attachment (id=27063) mozilla/mailnews/addrbook/
r=edburns, attachment (id=27050) mozilla/modules/oji/
attachment (id=27055) mozilla/layout/svg/
discontinued:
veto=jst, attachment (id=27045) mozilla/dom/
veto=dr, attachment (id=27064) mozilla/mailnews/mime/
veto=morse, attachment (id=27057) mozilla/extensions/cookie/
*s were addressed in the followups and seemed to satisfy their requestors.

I addressed all of shaver's concerns except for * mailnews/addrbook: which 
would require a considerable rewrite and would make this already long bug even 
longer. [I will file a bug to do that as soon as this lands]

Comment 52

[Dan Rosen]

Cool! So when are you going to land this baby? You should get brendan or
somebody to rs= those patches which haven't been sr='ed yet...

Comment 53

[Dan Rosen]

Uhh, did I say rs=? I meant sr= of course...

Comment 54

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

sr=shaver.  Get in the tree tonight?

Comment 55

[timeless]

fixes checked in. now i'll sit on hook for a month.

Comment 56

[timeless]

According to shrike warnings dropped by 90 around the time of this checkin 
woohoo :)

filed Bug 72781 rewrite getter_AddRefs(pAddressLists->ElementAt(j)) using 
QueryElementAt 

Comment 57

[karnaze (gone)]

*** Bug 69762 has been marked as a duplicate of this bug. ***