1238605 - Compiler warning: unused |foundServer| variable and removing extra check

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: shutup-rv-maybe-uninitialized-warning-nsMsgAccountManager.patch

I noticed a compiler warning when mailnews/base/src/nsMsgAccountManager.cpp is compiled:

A variable |foundServer| is no longer used.
Also when I checked the source, I noticed an extra check
of |sBundleService|. A check is performed immediately before |if ( sBundleService)| by |   NS_ENSURE_TRUE(sBundleService, NS_ERROR_UNEXPECTED);| so if-check is unnecessary.

The attached patch fixes the issue and removes the compiler warning.

TIA

Comment 1

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8706421 [details] [diff] [review]
shutup-rv-maybe-uninitialized-warning-nsMsgAccountManager.patch

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

Looks good to me.  I'm assuming all the tests pass with it removed?

Comment 2

[:aceman]

I have the variable removal in bug https://bug1232107.bmoattachments.org/attachment.cgi?id=8698658.
But the unneded condition is a good catch.

But if you can get the review here sooner than me, I'll drop that part from my patch. Just please do not file bugs for the remaining warnings, if they are covered by my patch :)

Comment 3

[ISHIKAWA, Chiaki]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #1)
> Comment on attachment 8706421 [details] [diff] [review]
> shutup-rv-maybe-uninitialized-warning-nsMsgAccountManager.patch
> 
> Review of attachment 8706421 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me.  I'm assuming all the tests pass with it removed?

Yes. I checked my local patches on linux on a local PC.

As for OSX, and Windows, I rely on tryserver.
However, lately tryserver jobs fail often due to underlying network issues (I have no idea how a contributor can alert this issue to those  who are in charge).

Anyway, luckily, a latest tryserver run did compile OSX and Windows, and so presumably
it is OK. (I have to say this in a half-hearted manner because between the last time tryserver build succeeds and
the latest tryserver build, a set of new errors seem to be introduced, e.g. mozmill and strange OS X memory errors (crashes).)

But as far as compilation is concerned, yes it works just fine and local linux test works as expected (without introducing new errors).

(In reply to :aceman from comment #2)
> I have the variable removal in bug
> https://bug1232107.bmoattachments.org/attachment.cgi?id=8698658.
> But the unneded condition is a good catch.
> 
> But if you can get the review here sooner than me, I'll drop that part from
> my patch. Just please do not file bugs for the remaining warnings, if they
> are covered by my patch :)

Aceman, I did not know that you were doing such cleanup of C-C TB sources.
I will simply get this patch go in.
I won't file new other bugs, but when I filed three compilation warning patches including this bugzilla entry, the other ones are 

Bug 1238615 - Compiler warning: possibly uninitialized |rv| - mailnews/base/src/nsMsgPrintEngine.cpp 
   and this is also part of your patch.

Bug 1238608 - Compiler warning: possibly uninitialized |rv| - mailnews/base/src/nsMessenger.cpp
   and this is also part of your patch (but I think the fix suggested is different in Bug 1238609.)

What would we want to synchrnize the patches here?

TIA

Comment 4

[:aceman]

(In reply to ISHIKAWA, Chiaki from comment #3)
> Bug 1238615 - Compiler warning: possibly uninitialized |rv| -
> mailnews/base/src/nsMsgPrintEngine.cpp 
>    and this is also part of your patch.
> 
> Bug 1238608 - Compiler warning: possibly uninitialized |rv| -
> mailnews/base/src/nsMessenger.cpp
>    and this is also part of your patch (but I think the fix suggested is
> different in Bug 1238609.)
> 
> What would we want to synchrnize the patches here?

Thanks, I think your versions are better. It will be great if you get them reviewed sooner by someone else and get them landed.
I wait for jcranmer specifically as there are some non-trivial changes in my patch.

Comment 5

[:aceman]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #1)
> Looks good to me.  I'm assuming all the tests pass with it removed?

Pushed to try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=524113749ab4

Comment 6

[:aceman]

Another try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=94e3acf94fed

If this passes, I'll check in the patch.

Comment 7

[:aceman]

The tests have passed. Chiaki, can I check it in?

Comment 8

[ISHIKAWA, Chiaki]

Please do.

BTW, I have been rather surprised that there have been so many build failures with C-C tryserver jobs. I wonder what is going on.
(See bug 1241762 for example, but there seems to be other problems. Although I can test linux build and run mozmill and xpcshell-tests locally on my linux PC, C-C tryserver does not seem to have build linux binries for the last three weeks or so?)
But I digress.

TIA

Comment 9

[:aceman]

Checked in:
https://hg.mozilla.org/comm-central/rev/e3b119d6545a

Comment 10

[:aceman]

(In reply to ISHIKAWA, Chiaki from comment #8)
> Please do.
> 
> BTW, I have been rather surprised that there have been so many build
> failures with C-C tryserver jobs. I wonder what is going on.
> I can test linux build and run mozmill and xpcshell-tests locally on my
> linux PC, C-C tryserver does not seem to have build linux binries for the
> last three weeks or so?)

Yes, the failures are also on the main trunk server. Often m-c breaks us with API changes. Then the linux problems seem to be some problems on the server itself that need to be fixed. It is normal that at those times the build runs fine locally (it does for me too).

But at least we have now fixed all the compiler warnings in /mailnews, at least with gcc5.3 on linux 32 bit.
Or do you see any more and have you bugs filed for them?

Comment 11

[ISHIKAWA, Chiaki]

Thank you for check-in.

(In reply to :aceman from comment #10)
> 
> But at least we have now fixed all the compiler warnings in /mailnews, at
> least with gcc5.3 on linux 32 bit.
> Or do you see any more and have you bugs filed for them?

I am using gcc and noticed that warnings related to use of ERROR constants as CASE labeles.
E.g.:

/NREF-COMM-CENTRAL/comm-central/mailnews/compose/src/nsMsgSendReport.cpp: In member function ‘virtual nsresult nsMsgSendReport::DisplayReport(nsIPrompt*, bool, bool, nsresult*)’:
/NREF-COMM-CENTRAL/comm-central/mailnews/compose/src/nsMsgSendReport.cpp:285:7: warning: case value ‘2153066710’ not in enumerated type ‘nsresult’ [-Wswitch]
       case NS_MSG_UNABLE_TO_SAVE_TEMPLATE:

and friends maybe a couple of dozens of lines or more.

I think this requires proper inclusion of NS_MSG_* error macros within nsresult ENUM type declaration (which did not happen since the error macro definition file in in M-C part and the maintainer did not want to include NS_MSG_* macros which Firefox doesn't need if I recall correctly.)

My new patch set may introduce a few more, and that is why I was checking warnings and removing them so that I am sure that my new patch set does not introduce any new ones.

TIA

Comment 12

[:aceman]

(In reply to ISHIKAWA, Chiaki from comment #11)
> /NREF-COMM-CENTRAL/comm-central/mailnews/compose/src/nsMsgSendReport.cpp: In
> member function ‘virtual nsresult nsMsgSendReport::DisplayReport(nsIPrompt*,
> bool, bool, nsresult*)’:
> /NREF-COMM-CENTRAL/comm-central/mailnews/compose/src/nsMsgSendReport.cpp:285:
> 7: warning: case value ‘2153066710’ not in enumerated type ‘nsresult’
> [-Wswitch]
>        case NS_MSG_UNABLE_TO_SAVE_TEMPLATE:
> 
> and friends maybe a couple of dozens of lines or more.
> 
> I think this requires proper inclusion of NS_MSG_* error macros within
> nsresult ENUM type declaration (which did not happen since the error macro
> definition file in in M-C part and the maintainer did not want to include
> NS_MSG_* macros which Firefox doesn't need if I recall correctly.)

Yes, that one is bug 783526.

So I do not see any more warnings aside that class of warnings.