605180 - Fix build warnings in netwerk/

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 484017 [details] [diff] [review]
Patch v1

Darin's not around anymore

Comment 2

[Jason Duell]

Comment on attachment 484017 [details] [diff] [review]
Patch v1

Great stuff--thanks for doing this.  I'm looking forward to necko being compiler-warning-free.  Only needs a few fixes.

Please add FAIL_ON_WARNINGS = 1 to all the Makefiles in the necko directories (see http://hg.mozilla.org/mozilla-central/rev/5da9fc2be835 for how to do this).  Then we need to run through tryserver to make sure we've caught all the errors on all platforms. (do you have tryserver access?  I can run it for you if you don't).


>diff --git a/netwerk/base/src/nsInputStreamPump.cpp b/netwerk/base/src/nsInputStreamPump.cpp
> 
>-        PRUint32 nextState;
>+        PRUint32 nextState = 0;
>         switch (mState) {
>         case STATE_START:
>             nextState = OnStateStart();
>             break;

Would adding a 'default' case with an NS_ASSERT and a 'return NS_ERROR_UNEXPECTED' also fix this?

>diff --git a/netwerk/base/src/nsSocketTransport2.cpp b/netwerk/base/src/nsSocketTransport2.cpp
>--- a/netwerk/base/src/nsSocketTransport2.cpp
>+++ b/netwerk/base/src/nsSocketTransport2.cpp
>@@ -283,34 +283,34 @@ nsSocketInputStream::Close()
> 
> NS_IMETHODIMP
> nsSocketInputStream::Available(PRUint32 *avail)
> {
>     LOG(("nsSocketInputStream::Available [this=%x]\n", this));
> 
>     *avail = 0;
> 
>-    PRFileDesc *fd;
>+    PRFileDesc* fd = NULL;
>     {
>         nsAutoLock lock(mTransport->mLock);
> 
>         if (NS_FAILED(mCondition))
>             return mCondition;
> 
>         fd = mTransport->GetFD_Locked();
>         if (!fd)
>             return NS_OK;
>     }

Change all uses of NULL to nsnull (or 0) per Mozilla Coding style guide (apparently NULL causes trouble on some platform):

    https://developer.mozilla.org/En/Developer_Guide/Coding_Style

Interesting that we unconditionally assign to fd, yet we get a use w/o init warning (I assume that's the warning): I guess some compiler is confused by the braces?

>diff --git a/netwerk/cache/nsCacheService.cpp b/netwerk/cache/nsCacheService.cpp
>@@ -1776,18 +1775,21 @@ nsCacheService::EnsureEntryHasDevice(nsC

For EnsureEntryHasDevice() let's instead just put 'nsresult rv;' at the top of the function, at full function scope.  We've got several real (non-debug) uses of it, which means we should be able to scrap all the '#if DEBUG' logic and still not get 'unused' warnings, AFAICT.

>diff --git a/netwerk/cache/nsDiskCacheBlockFile.h b/netwerk/cache/nsDiskCacheBlockFile.h
>--- a/netwerk/cache/nsDiskCacheBlockFile.h
>+++ b/netwerk/cache/nsDiskCacheBlockFile.h
>@@ -1,11 +1,10 @@
>-/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>- *
>- * ***** BEGIN LICENSE BLOCK *****
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****

Please add canonical vim modeline from Mozilla Style Guide to this file, too; 

    /* vim: set sw=4 ts=8 et tw=80 : */

If there are any other missing vim modeline in any other files, feel free to add them--that'd be a help.

>diff --git a/netwerk/cookie/nsCookieService.cpp b/netwerk/cookie/nsCookieService.cpp

Question:  for all these repeated instances of

    result rv = FunctionCall(...)
    NS_ASSERT_SUCCESS(rv);

The solution you're got here:

    #ifdef DEBUG
      nsresult rv =
    #endif
    FunctionCall(...)
    NS_ASSERT_SUCCESS(rv);

is clunky, and I'd like to avoid it if possible by some dummy use of rv.  If there's a non-debug use of rv anywhere in the function we can do this by moving rv to function-wide scope (as mentioned above).  Otherwise (like for the functions in this file), I'm hoping we can shut the compiler up with something like

    // force use of rv to avoid 'unused' warnings for !DEBUG
    if (NS_FAILED(rv))
        NS_ASSERT_SUCCESS(rv);

If !DEBUG, the body of the if is essentially empty, but it's enough to shut gcc up, at least.  Let's try it and see if it works for devstudio too. (Note: you only need one instance of this per function: keep the other NS_ASSERT_SUCCESS's unchanged).


>diff --git a/netwerk/mime/nsMIMEHeaderParamImpl.cpp b/netwerk/mime/nsMIMEHeaderParamImpl.cpp
>-/* vim:expandtab:shiftwidth=2:tabstop=4:
>- */
>+/* vim:expandtab:shiftwidth=2:tabstop=4: */

This vim modeline seems to make my vim unhappy.  From a glance at the vim help, it looks like the version of the modelines that accepts non-option text (like '*/') at the end of the string also requires 'set'.  Just use the canonical vim modeline (which I pasted in above).

>diff --git a/netwerk/protocol/http/PHttpChannelParams.h b/netwerk/protocol/http/PHttpChannelParams.h
>--- a/netwerk/protocol/http/PHttpChannelParams.h
>+++ b/netwerk/protocol/http/PHttpChannelParams.h
>@@ -1,28 +1,27 @@
> /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> /* vim: set sw=2 ts=8 et tw=80 : */
>-
> /* ***** BEGIN LICENSE BLOCK *****
> 
>  * The Initial Developer of the Original Code is
>- *  The Mozilla Foundation
>+ * The Mozilla Foundation

Getting rid of the blank line is fine, but I wouldn't bother trying to realign every instance of an initial developer that's indented.  Doesn't really matter.

>diff --git a/netwerk/streamconv/converters/nsDirIndexParser.cpp b/netwerk/streamconv/converters/nsDirIndexParser.cpp
>--- a/netwerk/streamconv/converters/nsDirIndexParser.cpp
>+++ b/netwerk/streamconv/converters/nsDirIndexParser.cpp
>@@ -187,17 +187,17 @@ nsDirIndexParser::ParseFormat(const char
>   unsigned int num = 0;
>   do {
>     while (*pos && nsCRT::IsAsciiSpace(PRUnichar(*pos)))
>       ++pos;
>     
>     ++num;
>     // There are a maximum of six allowed header fields (doubled plus
>     // terminator, just in case) -- Bug 443299
>-    if (num > (2 * NS_ARRAY_LENGTH(gFieldTable)))
>+    if (num > int(2 * NS_ARRAY_LENGTH(gFieldTable)))

So I assume the compiler's complaining about comparing the result of a sizeof with an unsigned int (num)?  (g++ -Wall doesn't give me this warning)   I'm surprised that it's happier comparing an unsigned int with a int.  It'd be better to see the exact same type here (unsigned int vs unsigned int).  Probably better to use static_cast<unsigned int>, too.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

(In reply to comment #2)
> Comment on attachment 484017 [details] [diff] [review]
> Patch v1
> 
> Great stuff--thanks for doing this.  I'm looking forward to necko being
> compiler-warning-free.  Only needs a few fixes.
> 
> Please add FAIL_ON_WARNINGS = 1 to all the Makefiles in the necko directories
> (see http://hg.mozilla.org/mozilla-central/rev/5da9fc2be835 for how to do
> this).  Then we need to run through tryserver to make sure we've caught all the
> errors on all platforms. (do you have tryserver access?  I can run it for you
> if you don't).

<http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/643e944f75e3bae4/fdcaa28201910ed9> suggests not to.

> >diff --git a/netwerk/base/src/nsInputStreamPump.cpp b/netwerk/base/src/nsInputStreamPump.cpp
> > 
> >-        PRUint32 nextState;
> >+        PRUint32 nextState = 0;
> >         switch (mState) {
> >         case STATE_START:
> >             nextState = OnStateStart();
> >             break;
> 
> Would adding a 'default' case with an NS_ASSERT and a 'return
> NS_ERROR_UNEXPECTED' also fix this?

Done.

> >diff --git a/netwerk/base/src/nsSocketTransport2.cpp b/netwerk/base/src/nsSocketTransport2.cpp
> >--- a/netwerk/base/src/nsSocketTransport2.cpp
> >+++ b/netwerk/base/src/nsSocketTransport2.cpp
> >@@ -283,34 +283,34 @@ nsSocketInputStream::Close()
> > 
> > NS_IMETHODIMP
> > nsSocketInputStream::Available(PRUint32 *avail)
> > {
> >     LOG(("nsSocketInputStream::Available [this=%x]\n", this));
> > 
> >     *avail = 0;
> > 
> >-    PRFileDesc *fd;
> >+    PRFileDesc* fd = NULL;
> >     {
> >         nsAutoLock lock(mTransport->mLock);
> > 
> >         if (NS_FAILED(mCondition))
> >             return mCondition;
> > 
> >         fd = mTransport->GetFD_Locked();
> >         if (!fd)
> >             return NS_OK;
> >     }
> 
> Change all uses of NULL to nsnull (or 0) per Mozilla Coding style guide
> (apparently NULL causes trouble on some platform):
> 
>     https://developer.mozilla.org/En/Developer_Guide/Coding_Style

It doesn't cause any trouble, at least not anymore. It's being used all over the IPC code, in particular. I'll change if you like, though.

> Interesting that we unconditionally assign to fd, yet we get a use w/o init
> warning (I assume that's the warning): I guess some compiler is confused by the
> braces?

Yeah, my compiler isn't the smartest one around.

> >diff --git a/netwerk/cache/nsCacheService.cpp b/netwerk/cache/nsCacheService.cpp
> >@@ -1776,18 +1775,21 @@ nsCacheService::EnsureEntryHasDevice(nsC
> 
> For EnsureEntryHasDevice() let's instead just put 'nsresult rv;' at the top of
> the function, at full function scope.  We've got several real (non-debug) uses
> of it, which means we should be able to scrap all the '#if DEBUG' logic and
> still not get 'unused' warnings, AFAICT.

Good point.

> >diff --git a/netwerk/cache/nsDiskCacheBlockFile.h b/netwerk/cache/nsDiskCacheBlockFile.h
> >--- a/netwerk/cache/nsDiskCacheBlockFile.h
> >+++ b/netwerk/cache/nsDiskCacheBlockFile.h
> >@@ -1,11 +1,10 @@
> >-/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> >- *
> >- * ***** BEGIN LICENSE BLOCK *****
> >+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> >+/* ***** BEGIN LICENSE BLOCK *****
> 
> Please add canonical vim modeline from Mozilla Style Guide to this file, too; 
> 
>     /* vim: set sw=4 ts=8 et tw=80 : */
> 
> If there are any other missing vim modeline in any other files, feel free to
> add them--that'd be a help.
> 
> >diff --git a/netwerk/cookie/nsCookieService.cpp b/netwerk/cookie/nsCookieService.cpp
> 
> Question:  for all these repeated instances of
> 
>     result rv = FunctionCall(...)
>     NS_ASSERT_SUCCESS(rv);
> 
> The solution you're got here:
> 
>     #ifdef DEBUG
>       nsresult rv =
>     #endif
>     FunctionCall(...)
>     NS_ASSERT_SUCCESS(rv);
> 
> is clunky, and I'd like to avoid it if possible by some dummy use of rv.  If
> there's a non-debug use of rv anywhere in the function we can do this by moving
> rv to function-wide scope (as mentioned above).  Otherwise (like for the
> functions in this file), I'm hoping we can shut the compiler up with something
> like
> 
>     // force use of rv to avoid 'unused' warnings for !DEBUG
>     if (NS_FAILED(rv))
>         NS_ASSERT_SUCCESS(rv);
> 
> If !DEBUG, the body of the if is essentially empty, but it's enough to shut gcc
> up, at least.  Let's try it and see if it works for devstudio too. (Note: you
> only need one instance of this per function: keep the other NS_ASSERT_SUCCESS's
> unchanged).

Okay, that works too.

> >diff --git a/netwerk/mime/nsMIMEHeaderParamImpl.cpp b/netwerk/mime/nsMIMEHeaderParamImpl.cpp
> >-/* vim:expandtab:shiftwidth=2:tabstop=4:
> >- */
> >+/* vim:expandtab:shiftwidth=2:tabstop=4: */
> 
> This vim modeline seems to make my vim unhappy.  From a glance at the vim help,
> it looks like the version of the modelines that accepts non-option text (like
> '*/') at the end of the string also requires 'set'.  Just use the canonical vim
> modeline (which I pasted in above).

Done.

> >diff --git a/netwerk/protocol/http/PHttpChannelParams.h b/netwerk/protocol/http/PHttpChannelParams.h
> >--- a/netwerk/protocol/http/PHttpChannelParams.h
> >+++ b/netwerk/protocol/http/PHttpChannelParams.h
> >@@ -1,28 +1,27 @@
> > /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> > /* vim: set sw=2 ts=8 et tw=80 : */
> >-
> > /* ***** BEGIN LICENSE BLOCK *****
> > 
> >  * The Initial Developer of the Original Code is
> >- *  The Mozilla Foundation
> >+ * The Mozilla Foundation
> 
> Getting rid of the blank line is fine, but I wouldn't bother trying to realign
> every instance of an initial developer that's indented.  Doesn't really matter.

Sure.

> >diff --git a/netwerk/streamconv/converters/nsDirIndexParser.cpp b/netwerk/streamconv/converters/nsDirIndexParser.cpp
> >--- a/netwerk/streamconv/converters/nsDirIndexParser.cpp
> >+++ b/netwerk/streamconv/converters/nsDirIndexParser.cpp
> >@@ -187,17 +187,17 @@ nsDirIndexParser::ParseFormat(const char
> >   unsigned int num = 0;
> >   do {
> >     while (*pos && nsCRT::IsAsciiSpace(PRUnichar(*pos)))
> >       ++pos;
> >     
> >     ++num;
> >     // There are a maximum of six allowed header fields (doubled plus
> >     // terminator, just in case) -- Bug 443299
> >-    if (num > (2 * NS_ARRAY_LENGTH(gFieldTable)))
> >+    if (num > int(2 * NS_ARRAY_LENGTH(gFieldTable)))
> 
> So I assume the compiler's complaining about comparing the result of a sizeof
> with an unsigned int (num)?  (g++ -Wall doesn't give me this warning)   I'm
> surprised that it's happier comparing an unsigned int with a int.  It'd be
> better to see the exact same type here (unsigned int vs unsigned int). 
> Probably better to use static_cast<unsigned int>, too.

This hunk indeed doesn't make any sense, but I can't reproduce a warning either way. I'll just drop it.

I also fixed another uninitialized variable warning in nsCookieService::FindStaleCookie.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2.1

Merged to tip.

Comment 6

[Jason Duell]

Comment on attachment 492052 [details] [diff] [review]
Patch v2.1

Very close!  Will be +r with comments fixed.

>From: Ms2ger <[e-mail]>
>Bug 605180 - Fix build warnings in netwerk/; r?jduell
>
>diff --git a/netwerk/base/src/nsSocketTransport2.cpp b/netwerk/base/src/nsSocketTransport2.cpp
> 
>-    PRFileDesc *fd;
>+    PRFileDesc* fd = NULL;

Replace all uses of NULL with either nsnull or 0 (as per Style Guide: apparently NULL causes us grief on some platform).  I prefer '0', but can live with either.


>diff --git a/netwerk/cache/nsCacheService.cpp b/netwerk/cache/nsCacheService.cpp
>     nsresult res = NS_OK;
> 
> #ifdef NECKO_DISK_CACHE
>     if (storagePolicy == nsICache::STORE_ANYWHERE ||
>         storagePolicy == nsICache::STORE_ON_DISK) {
> 
>         if (mEnableDiskDevice) {
>-            nsresult rv;
>+            nsresult rv = NS_OK;
>             if (!mDiskDevice)
>                 rv = CreateDiskDevice();
>             if (mDiskDevice)
>                 rv = mDiskDevice->EvictEntries(clientID);
>-            if (NS_FAILED(rv)) res = rv;
>+            if (NS_FAILED(rv))
>+                res = rv;
>         }
>     }
> #endif // ! NECKO_DISK_CACHE
> 
> #ifdef NECKO_OFFLINE_CACHE
>     // Only clear the offline cache if it has been specifically asked for.
>     if (storagePolicy == nsICache::STORE_OFFLINE) {
>         if (mEnableOfflineDevice) {
>-            nsresult rv;
>+            nsresult rv = NS_OK;
>             if (!mOfflineDevice)
>                 rv = CreateOfflineDevice();
>             if (mOfflineDevice)
>                 rv = mOfflineDevice->EvictEntries(clientID);
>-            if (NS_FAILED(rv)) res = rv;
>+            if (NS_FAILED(rv))
>+                res = rv;
>         }
>     }
> #endif // ! NECKO_OFFLINE_CACHE
> 
>     if (storagePolicy == nsICache::STORE_ANYWHERE ||
>         storagePolicy == nsICache::STORE_IN_MEMORY) {
>-
>         // If there is no memory device, there is no need to evict it...
>         if (mMemoryDevice) {
>-            nsresult rv;
>-            rv = mMemoryDevice->EvictEntries(clientID);
>+            nsresult rv = mMemoryDevice->EvictEntries(clientID);
>             if (NS_FAILED(rv)) res = rv;


Do we need all the nsresult declarations in the nested if blocks, given that we've got nsresult declared already at the top of the diff context?  I hate masking the scope of a function-level variable by using same-named block variables--error prone.

>diff --git a/netwerk/cookie/nsCookieService.cpp b/netwerk/cookie/nsCookieService.cpp

>-  nsresult rv = mDefaultDBState->pendingRead->Cancel();
>+#ifdef DEBUG
>+  nsresult rv =
>+#endif
>+  mDefaultDBState->pendingRead->Cancel();

We finally have a better way to do this!   Replace with DebugOnly<nsresult> rv.  See bug 656992.


>@@ -3631,17 +3634,19 @@ nsCookieService::RemoveCookieFromList(co
>       stmt->NewBindingParamsArray(getter_AddRefs(paramsArray));
>     }
> 
>     nsCOMPtr<mozIStorageBindingParams> params;
>     paramsArray->NewBindingParams(getter_AddRefs(params));
> 
>     nsresult rv = params->BindUTF8StringByName(NS_LITERAL_CSTRING("name"),
>                                                aIter.Cookie()->Name());
>-    NS_ASSERT_SUCCESS(rv);
>+    if (NS_FAILED(rv)) {
>+      NS_ASSERT_SUCCESS(rv);
>+    }

I don't get why we need the NS_FAILED here--NS_ASSERT_SUCCESS already contains an NS_FAILED call.   This can probably be fixed with DebugOnly instead?

Comment 7

[Jason Duell]

Now that we've turned on --enable-warnings-as-errors for Linux/Max build bots, this bug is worth revisiting/finishing/unbitrotting.  Ms2ger, are you still interested?

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I'll try to get to it soonish, thanks for the reminder.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v3

Finally got around to updating this.

Comment 10

[Jason Duell]

Comment on attachment 613139 [details] [diff] [review]
Patch v3

Review of attachment 613139 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Only one nit.

So I see you've added FAIL_ON_WARNINGS to 7 of the necko makefiles.  It's great to have that on, but I'm wondering about scope. Did you decide to hold off on the other Makefile.in's (including big ones like protocol/http) because that's a followup, or some other reason?

Have you run this patch through try?  We should definitely do that before we land it.

::: netwerk/mime/nsMIMEHeaderParamImpl.cpp
@@ +1,2 @@
>  /* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set sw=4 ts=8 et tw=80 : */

This file is a mix of 2 and 4 space indents, but more of it is 2-space.  So let's keep sw=2 here.  Rest of modeline is ok.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Jason Duell (:jduell) from comment #10)
> Comment on attachment 613139 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 613139 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.  Only one nit.
> 
> So I see you've added FAIL_ON_WARNINGS to 7 of the necko makefiles.  It's
> great to have that on, but I'm wondering about scope. Did you decide to hold
> off on the other Makefile.in's (including big ones like protocol/http)
> because that's a followup, or some other reason?

These grew organically... I'll get this patch landed, and file a followup to see if we can enable it for all of necko, if that's OK with you.

> Have you run this patch through try?  We should definitely do that before we
> land it.

Yep: https://tbpl.mozilla.org/?tree=Try&rev=a3d07d5de73b

Comment 12

[Jason Duell]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfa27d58769d

Comment 13

[Nicholas Nethercote [inactive]]

> > So I see you've added FAIL_ON_WARNINGS to 7 of the necko makefiles.

Woo!

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Thanks, Jason!

https://hg.mozilla.org/mozilla-central/rev/bfa27d58769d