Clean up lots of "shadows" and other common warnings

RESOLVED FIXED in mozilla0.9

Status

SeaMonkey
General
RESOLVED FIXED
17 years ago
13 years ago

People

(Reporter: timeless, Assigned: timeless)

Tracking

Trunk
mozilla0.9
x86
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(35 attachments)

51.72 KB, patch
Details | Diff | Splinter Review
3.62 KB, patch
Details | Diff | Splinter Review
1.83 KB, patch
Details | Diff | Splinter Review
2.11 KB, patch
Details | Diff | Splinter Review
1.90 KB, patch
Details | Diff | Splinter Review
624 bytes, patch
Details | Diff | Splinter Review
669 bytes, patch
Details | Diff | Splinter Review
3.25 KB, patch
Details | Diff | Splinter Review
644 bytes, patch
Details | Diff | Splinter Review
663 bytes, patch
Details | Diff | Splinter Review
7.37 KB, patch
Details | Diff | Splinter Review
13.34 KB, patch
Details | Diff | Splinter Review
1.20 KB, patch
Details | Diff | Splinter Review
889 bytes, patch
Details | Diff | Splinter Review
794 bytes, patch
Details | Diff | Splinter Review
1.88 KB, patch
Details | Diff | Splinter Review
744 bytes, patch
Details | Diff | Splinter Review
4.65 KB, patch
Details | Diff | Splinter Review
612 bytes, patch
Details | Diff | Splinter Review
3.31 KB, patch
Details | Diff | Splinter Review
4.80 KB, patch
Details | Diff | Splinter Review
805 bytes, patch
Details | Diff | Splinter Review
435 bytes, patch
Details | Diff | Splinter Review
269 bytes, patch
Details | Diff | Splinter Review
755 bytes, patch
Details | Diff | Splinter Review
1017 bytes, patch
Details | Diff | Splinter Review
159 bytes, patch
Details | Diff | Splinter Review
160 bytes, patch
Details | Diff | Splinter Review
743 bytes, patch
Details | Diff | Splinter Review
720 bytes, patch
Details | Diff | Splinter Review
2.29 KB, patch
Details | Diff | Splinter Review
750 bytes, patch
Details | Diff | Splinter Review
1.60 KB, patch
Details | Diff | Splinter Review
674 bytes, patch
Details | Diff | Splinter Review
49.86 KB, patch
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

17 years ago
Created attachment 26863 [details] [diff] [review]
patch 1
(Assignee)

Updated

17 years ago
Blocks: 70386
Status: NEW → ASSIGNED
Keywords: approval, patch, review

Comment 2

17 years ago
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.
Component: Tracking → Browser-General
QA Contact: chofmann → dr
Summary: timeless promised to clean up some warnings → Clean up lots of "shadows" and other common warnings
(Assignee)

Comment 3

17 years ago
Created attachment 27044 [details] [diff] [review]
mozilla/content
(Assignee)

Comment 4

17 years ago
Created attachment 27045 [details] [diff] [review]
mozilla/dom/
(Assignee)

Comment 5

17 years ago
Created attachment 27046 [details] [diff] [review]
mozilla/uriloader/
(Assignee)

Comment 6

17 years ago
Created attachment 27047 [details] [diff] [review]
mozilla/view/
(Assignee)

Comment 7

17 years ago
Created attachment 27048 [details] [diff] [review]
mozilla/gfx/
(Assignee)

Comment 8

17 years ago
Created attachment 27049 [details] [diff] [review]
mozilla/xpcom/
(Assignee)

Comment 9

17 years ago
Created attachment 27050 [details] [diff] [review]
mozilla/modules/oji
(Assignee)

Comment 10

17 years ago
Created attachment 27051 [details] [diff] [review]
mozilla/embedding/
(Assignee)

Comment 11

17 years ago
Created attachment 27052 [details] [diff] [review]
mozilla/webshell/
(Assignee)

Comment 12

17 years ago
Created attachment 27053 [details] [diff] [review]
mozilla/layout/xul/
(Assignee)

Comment 13

17 years ago
Created attachment 27054 [details] [diff] [review]
mozilla/layout/html/
(Assignee)

Comment 14

17 years ago
Created attachment 27055 [details] [diff] [review]
mozilla/layout/svg/
(Assignee)

Comment 15

17 years ago
Created attachment 27056 [details] [diff] [review]
mozilla/layout/base/
(Assignee)

Comment 16

17 years ago
Created attachment 27057 [details] [diff] [review]
mozilla/extensions/cookie/
(Assignee)

Comment 17

17 years ago
Created attachment 27059 [details] [diff] [review]
mozilla/mailnews/base/
(Assignee)

Comment 18

17 years ago
Created attachment 27060 [details] [diff] [review]
mozilla/mailnews/db/
(Assignee)

Comment 19

17 years ago
Created attachment 27061 [details] [diff] [review]
mozilla/mailnews/imap/
(Assignee)

Comment 20

17 years ago
Created attachment 27062 [details] [diff] [review]
mozilla/mailnews/local/
(Assignee)

Comment 21

17 years ago
Created attachment 27063 [details] [diff] [review]
mozilla/mailnews/addrbook/
(Assignee)

Comment 22

17 years ago
Created attachment 27064 [details] [diff] [review]
mozilla/mailnews/mime/
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 26

17 years ago
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!
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.
(Assignee)

Comment 28

17 years ago
Created attachment 27859 [details] [diff] [review]
mozilla/extensions/cookie/ (updated)
(Assignee)

Comment 29

17 years ago
Created attachment 27860 [details] [diff] [review]
mozilla/content/ (patch per jst)
(Assignee)

Comment 30

17 years ago
Created attachment 27861 [details] [diff] [review]
mozilla/layout/html/ (patch per jst)

Comment 31

17 years ago
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?)
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
(Assignee)

Comment 33

17 years ago
Created attachment 27913 [details] [diff] [review]
mozilla/mailnews/addrbook/ (patch per ducarroz)
(Assignee)

Comment 34

17 years ago
Created attachment 27914 [details] [diff] [review]
mozilla/mailnews/imap/ (patch per ducarroz)
R=ducarroz for all mailnews patches. Thanks for doing this clean up!

Comment 36

17 years ago
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

17 years ago
Oops, I meant to say bug 46783 rather than 27859 above.  The latter is your 
attachment number.
(Assignee)

Updated

17 years ago
Target Milestone: --- → mozilla0.9

Comment 38

17 years ago
r=edburns for modules/oji
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).
(Assignee)

Comment 40

17 years ago
Created attachment 28103 [details] [diff] [review]
mozilla/view/ (patch per dbaron)
(Assignee)

Comment 41

17 years ago
Created attachment 28104 [details] [diff] [review]
mozilla/xpcom/ (patch per dbaron)
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.

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

Comment 44

17 years ago
Created attachment 28259 [details] [diff] [review]
mozilla/xpcom/ (revised patch per shaver)
(Assignee)

Comment 45

17 years ago
Created attachment 28260 [details] [diff] [review]
mozilla/webshell (patch per shaver)
(Assignee)

Comment 46

17 years ago
Created attachment 28261 [details] [diff] [review]
mozilla/mailnews/addrbook (revised patch per shaver)
(Assignee)

Comment 47

17 years ago
Created attachment 28262 [details] [diff] [review]
mozilla/mailnews/base (patch per shaver)
(Assignee)

Comment 48

17 years ago
Created attachment 28263 [details] [diff] [review]
mozilla/layout/xul (patch per shaver)
(Assignee)

Comment 49

17 years ago
Created attachment 28264 [details] [diff] [review]
mozilla/layout/svg (patch per shaver)
(Assignee)

Comment 50

17 years ago
Created attachment 28327 [details] [diff] [review]
collected+corrected patch
(Assignee)

Comment 51

17 years ago
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

17 years ago
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

17 years ago
Uhh, did I say rs=? I meant sr= of course...
sr=shaver.  Get in the tree tonight?
(Assignee)

Comment 55

17 years ago
fixes checked in. now i'll sit on hook for a month.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
(Assignee)

Comment 56

17 years ago
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 
Keywords: approval, review

Comment 57

17 years ago
*** Bug 69762 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.