Last Comment Bug 796085 - fix some "warning: variable 'rv' set but not used [-Wunused-but-set-variable]" gcc warnings in /mailnews
: fix some "warning: variable 'rv' set but not used [-Wunused-but-set-variable]...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: Thunderbird 19.0
Assigned To: :aceman
:
:
Mentors:
Depends on:
Blocks: buildwarning
  Show dependency treegraph
 
Reported: 2012-10-01 12:52 PDT by :aceman
Modified: 2012-10-22 16:56 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
1base (8.84 KB, patch)
2012-10-01 14:45 PDT, :aceman
neil: review+
Details | Diff | Splinter Review
2compose (5.01 KB, patch)
2012-10-01 14:46 PDT, :aceman
no flags Details | Diff | Splinter Review
3db (5.46 KB, patch)
2012-10-01 14:46 PDT, :aceman
irving: review-
Details | Diff | Splinter Review
[checked in] 4smime (2.00 KB, patch)
2012-10-01 14:47 PDT, :aceman
irving: review+
Details | Diff | Splinter Review
5import (4.09 KB, patch)
2012-10-01 14:47 PDT, :aceman
standard8: review-
Details | Diff | Splinter Review
6news (5.08 KB, patch)
2012-10-01 14:48 PDT, :aceman
Pidgeot18: review+
Details | Diff | Splinter Review
[checked in] 7imap (3.12 KB, patch)
2012-10-01 14:48 PDT, :aceman
irving: review+
Details | Diff | Splinter Review
[checked in] 6news v2 (5.20 KB, patch)
2012-10-02 10:37 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review
5import v2 (5.37 KB, patch)
2012-10-03 11:33 PDT, :aceman
standard8: review+
Details | Diff | Splinter Review
[checked in ] 1base v2 (8.76 KB, patch)
2012-10-08 11:07 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review
[checked in] 2compose v2 (4.95 KB, patch)
2012-10-09 11:31 PDT, :aceman
neil: review+
Details | Diff | Splinter Review
5import v3 (5.32 KB, patch)
2012-10-09 11:37 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review
[checked in] 5import v4 (5.37 KB, patch)
2012-10-10 10:58 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review
[checked in] 3db v2 (5.54 KB, patch)
2012-10-22 12:12 PDT, :aceman
irving: review+
Details | Diff | Splinter Review

Description :aceman 2012-10-01 12:52:45 PDT
Building with gcc 4.7 found these:

mailnews/base/src/nsMsgAccountManager.cpp:1814:12: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
mailnews/base/src/nsMsgAccountManagerDS.cpp:844:12: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
mailnews/base/src/nsMsgDBView.cpp:6807:12: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
mailnews/base/src/nsMsgDBView.cpp:7055:14: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
mailnews/base/src/nsMsgPurgeService.cpp:474:12: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
mailnews/base/util/nsMsgDBFolder.cpp:1026:12: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
mailnews/base/util/nsMsgIncomingServer.cpp:1589:12: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
mailnews/compose/src/nsMsgPrompts.cpp:104:12: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
mailnews/compose/src/nsMsgPrompts.cpp:86:12: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
mailnews/compose/src/nsURLFetcher.cpp:245:12: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
mailnews/db/msgdb/src/nsMsgDatabase.cpp:255:16: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
mailnews/db/msgdb/src/nsMsgDatabase.cpp:5096:16: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
mailnews/extensions/smime/src/nsMsgComposeSecure.cpp:199:12: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
mailnews/import/src/nsImportAddressBooks.cpp:444:14: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
mailnews/import/src/nsImportAddressBooks.cpp:808:12: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
mailnews/import/src/nsImportMail.cpp:417:14: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
mailnews/local/src/nsLocalUndoTxn.cpp:38:14: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
mailnews/news/src/nsNewsDownloader.cpp:492:12: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
mailnews/news/src/nsNntpIncomingServer.cpp:277:12: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
mailnews/news/src/nsNntpIncomingServer.cpp:453:14: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
mailnews/news/src/nsNNTPProtocol.cpp:1750:16: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
mailnews/news/src/nsNNTPProtocol.cpp:4760:12: warning: variable 'rv' set but not used [-Wunused-but-set-variable]
Comment 1 :aceman 2012-10-01 13:26:44 PDT
I'll make several patches so that each module owner can review his part.
Comment 2 :aceman 2012-10-01 14:45:58 PDT
Created attachment 666721 [details] [diff] [review]
1base
Comment 3 :aceman 2012-10-01 14:46:30 PDT
Created attachment 666723 [details] [diff] [review]
2compose
Comment 4 :aceman 2012-10-01 14:46:55 PDT
Created attachment 666725 [details] [diff] [review]
3db
Comment 5 :aceman 2012-10-01 14:47:26 PDT
Created attachment 666727 [details] [diff] [review]
[checked in] 4smime
Comment 6 :aceman 2012-10-01 14:47:53 PDT
Created attachment 666728 [details] [diff] [review]
5import
Comment 7 :aceman 2012-10-01 14:48:19 PDT
Created attachment 666729 [details] [diff] [review]
6news
Comment 8 :aceman 2012-10-01 14:48:52 PDT
Created attachment 666730 [details] [diff] [review]
[checked in] 7imap
Comment 9 Joshua Cranmer [:jcranmer] 2012-10-01 15:08:12 PDT
Comment on attachment 666729 [details] [diff] [review]
6news

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

r+, if you fix the issue I mention:

::: mailnews/news/src/nsNNTPProtocol.cpp
@@ +4765,5 @@
>    // protocol - truly evil, but we're stuck at the moment.
> +  if (m_channelListener) {
> +    nsresult rv = m_channelListener->OnStopRequest(this, m_channelContext, NS_OK);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }

This method does not want an early return, so ignore the result of the call instead of propagating it (like the following if statement).
Comment 10 :aceman 2012-10-02 00:04:55 PDT
I have run xpcshell tests with the patches applied but they are inconclusive. They produced different failures t each run. I also get some failures on clean tree.

I did not check the rv randomly, I tried to look what the function does and whether it already handles failures in other cases. But it is quite possible that I have made some decisions wrong. So I ask the reviewers to check the logic and decide where I should not check the rv and change 'rv = ' to '(void)'. Like jcranmer did.

The changes of 'nsresult rv;' to 'nsresult rv = NS_OK;' are to fix 'variable 'rv' may be used uninitialized'. I looked at the flow and the compiler is right, there are patterns like:
nsresult rv;
if (something)
  rv = FunctionCall();

if (NS_SUCCEEDED(rv))
  ...

Of course I don't know if it can happen that 'something' is false but if it couldn't there would not have been the 'if' :)
Comment 11 :aceman 2012-10-02 10:37:37 PDT
Created attachment 667049 [details] [diff] [review]
[checked in] 6news v2
Comment 12 :aceman 2012-10-02 10:39:13 PDT
Ryan, is it possible to check-in each patch when it is done, without waiting for the others? The patches are spread over the tree and some of them can take longer so the bitrot chance is quite high.
Comment 13 :Irving Reid (No longer working on Firefox) 2012-10-02 13:35:51 PDT
Comment on attachment 666727 [details] [diff] [review]
[checked in] 4smime

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

Don't put r=whoever into the patch comment; the person checking the fix in will update the comment with the reviewer's name.
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-10-02 15:57:00 PDT
Comment on attachment 666727 [details] [diff] [review]
[checked in] 4smime

https://hg.mozilla.org/comm-central/rev/005b8cd8a647
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-10-02 15:57:32 PDT
Comment on attachment 667049 [details] [diff] [review]
[checked in] 6news v2

https://hg.mozilla.org/comm-central/rev/611934f581bf
Comment 16 Mark Banner (:standard8, limited time in Dec) 2012-10-03 10:24:42 PDT
Comment on attachment 666728 [details] [diff] [review]
5import

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

::: mailnews/import/src/nsImportAddressBooks.cpp
@@ +441,5 @@
>  
>    if (m_pBooks) {
>      uint32_t    count = 0;
> +    nsresult    rv = m_pBooks->Count(&count);
> +    NS_ENSURE_SUCCESS(rv, rv);

Its unlikely this will fail, but if it does, it'll just end up as 0. So I'd go for (void)m_pBooks->Count and drop the rv. Defaulting to zero is safe, as we won't do anything in the loop.

Bonus points for moving the totalSize definition inside the if statement.

@@ +458,2 @@
>            rv = book->GetSize(&size);
> +          NS_ENSURE_SUCCESS(rv, rv);

I'm not sure if import allows for a book of size zero, I think it might, so it might be safer just to let this through as it is at the moment.

::: mailnews/import/src/nsImportMail.cpp
@@ +417,5 @@
>      nsresult    rv;
>      uint32_t    size;
>  
>      rv = m_pMailboxes->Count(&count);
> +    NS_ENSURE_SUCCESS(rv, rv);

As per earlier, I think we can just drop the rv and check here.

@@ +430,2 @@
>            rv = box->GetSize(&size);
> +          NS_ENSURE_SUCCESS(rv, rv);

Ditto with the size.
Comment 17 :aceman 2012-10-03 11:33:09 PDT
Created attachment 667571 [details] [diff] [review]
5import v2
Comment 18 neil@parkwaycc.co.uk 2012-10-07 15:01:56 PDT
Comment on attachment 666721 [details] [diff] [review]
1base

>-      if (mSearchFolder)
>-        rv = mSearchFolder->DeleteMessages(mHdrsToDelete, nullptr, false /*delete storage*/, false /*isMove*/, nullptr, false /*allowUndo*/);
>+      if (mSearchFolder) {
>+        nsresult rv = mSearchFolder->DeleteMessages(mHdrsToDelete, nullptr, false /*delete storage*/, false /*isMove*/, nullptr, false /*allowUndo*/);
>+        NS_ENSURE_SUCCESS(rv, rv);
I don't like this change because it will prevent the listener from becoming unregistered. Since NotifyListenersDone ignores the result, you might as well ignore the result of deleting messages. r=me with that fixed.
Comment 19 :aceman 2012-10-08 11:07:45 PDT
Created attachment 669201 [details] [diff] [review]
[checked in ] 1base v2
Comment 20 neil@parkwaycc.co.uk 2012-10-08 16:22:23 PDT
Comment on attachment 666723 [details] [diff] [review]
2compose

> nsresult
> nsMsgAskBooleanQuestionByString(nsIPrompt * aPrompt, const PRUnichar * msg, bool *answer, const PRUnichar * windowTitle)
> {
>-  nsresult rv;
>+  nsresult rv = NS_OK;
>   nsCOMPtr<nsIPrompt> dialog = aPrompt;
> 
>-  if ((!msg) || (!*msg))
>-    return NS_ERROR_INVALID_ARG;
>+  NS_ENSURE_TRUE(msg && *msg, NS_ERROR_INVALID_ARG);
[As you're editing this, might as well move this up so it gets checked first.]


>-  if (mConverter)
>-    rv = mConverter->OnStopRequest(request, ctxt, aStatus);
>+  if (mConverter) {
>+    nsresult rv = mConverter->OnStopRequest(request, ctxt, aStatus);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
I'm worried about this change in behaviour. I'd prefer it if you just removed rv.
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-10-08 17:52:11 PDT
Comment on attachment 669201 [details] [diff] [review]
[checked in ] 1base v2

https://hg.mozilla.org/comm-central/rev/03fd0a59e409
Comment 22 Mark Banner (:standard8, limited time in Dec) 2012-10-09 03:28:33 PDT
Comment on attachment 667571 [details] [diff] [review]
5import v2

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

r=me with the one issue fixed.

::: mailnews/import/src/nsImportAddressBooks.cpp
@@ +810,5 @@
>  
>    nsString          success;
>    nsString          error;
> +  nsresult          rv = pData->books->Count(&count);
> +  NS_ENSURE_SUCCESS_VOID(rv);

I think you missed removing the rv here and just changing the Count call to ignore the return result.
Comment 23 :aceman 2012-10-09 03:37:15 PDT
(In reply to Mark Banner (:standard8) from comment #22)
> I think you missed removing the rv here and just changing the Count call to
> ignore the return result.

I always ADD checking if rv is there. Only if the reviewer says the return value can be ignored I remove the rv.
Comment 24 Mark Banner (:standard8, limited time in Dec) 2012-10-09 03:55:20 PDT
(In reply to :aceman from comment #23)
> (In reply to Mark Banner (:standard8) from comment #22)
> > I think you missed removing the rv here and just changing the Count call to
> > ignore the return result.
> 
> I always ADD checking if rv is there. Only if the reviewer says the return
> value can be ignored I remove the rv.

Ok, so basically this instance is the same as the other one in that file, and I wasn't explicit enough about the fact you needed to change that as well.
Comment 25 :aceman 2012-10-09 11:31:39 PDT
Sorry Mark, I now see what you mean. I have really removed the rv in some of the other functions in the same file. Now I wonder why, but it may be because those functions do not return in any other case so their callers may not expect it. So I went with the prevailing style of not checking anything. My comment 23 is invalid. I'll remove the one check I've added.
Comment 26 :aceman 2012-10-09 11:31:58 PDT
Created attachment 669648 [details] [diff] [review]
[checked in] 2compose v2
Comment 27 :aceman 2012-10-09 11:37:28 PDT
Created attachment 669649 [details] [diff] [review]
5import v3

Do I have a short memory :) The ignores of return value from Count() is due to your requests in comment 16 :)

So my comment was actually valid.
Comment 28 Ryan VanderMeulen [:RyanVM] 2012-10-09 17:42:12 PDT
Comment on attachment 669649 [details] [diff] [review]
5import v3

https://hg.mozilla.org/comm-central/rev/de7b9f15cd2e
Comment 29 Ryan VanderMeulen [:RyanVM] 2012-10-09 18:23:20 PDT
Comment on attachment 669649 [details] [diff] [review]
5import v3

Backed out for build bustage on all platforms.
https://hg.mozilla.org/comm-central/rev/d1c0ff3c2690

https://tbpl.mozilla.org/php/getParsedLog.php?id=15969550&tree=Thunderbird-Trunk

../../../../../mailnews/import/src/nsImportAddressBooks.cpp:822:7: error: use of undeclared identifier 'rv'
      rv = book->GetImport(&import);
      ^
../../../../../mailnews/import/src/nsImportAddressBooks.cpp:823:24: error: use of undeclared identifier 'rv'
      if (NS_SUCCEEDED(rv) && import)
                       ^
../../../../../mailnews/import/src/nsImportAddressBooks.cpp:824:9: error: use of undeclared identifier 'rv'
        rv = book->GetSize(&size);
        ^
../../../../../mailnews/import/src/nsImportAddressBooks.cpp:826:24: error: use of undeclared identifier 'rv'
      if (NS_SUCCEEDED(rv) && size && import) {
                       ^
../../../../../mailnews/import/src/nsImportAddressBooks.cpp:853:11: error: use of undeclared identifier 'rv'
          rv = pData->addressImport->ImportAddressBook(book,
          ^
../../../../../mailnews/import/src/nsImportAddressBooks.cpp:860:28: error: use of undeclared identifier 'rv'
          if (NS_SUCCEEDED(rv) && pSuccess) {
                           ^
6 errors generated.
make[9]: *** [nsImportAddressBooks.o] Error 1
Comment 30 :aceman 2012-10-10 10:58:34 PDT
Created attachment 670035 [details] [diff] [review]
[checked in] 5import v4

Sorry about that, try this.
Comment 31 Ryan VanderMeulen [:RyanVM] 2012-10-10 18:00:38 PDT
Comment on attachment 669648 [details] [diff] [review]
[checked in] 2compose v2

https://hg.mozilla.org/comm-central/rev/2bfa5fb694bf
Comment 32 Ryan VanderMeulen [:RyanVM] 2012-10-10 18:01:09 PDT
Comment on attachment 670035 [details] [diff] [review]
[checked in] 5import v4

https://hg.mozilla.org/comm-central/rev/9c4f3220eff9
Comment 33 :Irving Reid (No longer working on Firefox) 2012-10-22 09:46:35 PDT
Comment on attachment 666725 [details] [diff] [review]
3db

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

The choice of which errors to handle and which to ignore in the original code doesn't make much sense to me. Based on that, I'd prefer that this patch not change behaviour unless you want to take the time to analyze all the calls and figure out which failures make sense and what the right way to recover from those failures is.

So, I'd prefer to see the msgDatabase->m_folder->GetFilePath() and GetSummaryFileLocation() calls cast to (void) in this patch.

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +253,3 @@
>        msgDatabase->m_thumb = nullptr;
>        nsCOMPtr<nsIFile> folderPath;
> +      rv = msgDatabase->m_folder->GetFilePath(getter_AddRefs(folderPath));

Slight behaviour change - we used to ignore this result (which should almost never return NS_FAILED, even when it fails)

@@ +256,2 @@
>        nsCOMPtr <nsIFile> summaryFile;
>        rv = GetSummaryFileLocation(folderPath, getter_AddRefs(summaryFile));

also used to ignore this result

@@ +256,4 @@
>        nsCOMPtr <nsIFile> summaryFile;
>        rv = GetSummaryFileLocation(folderPath, getter_AddRefs(summaryFile));
>  
> +      if (NS_SUCCEEDED(rv))

This if is now based on the result of GetSummaryFileLocation, it used to be based on ThumbToOpenStore
Comment 34 :aceman 2012-10-22 12:12:47 PDT
Created attachment 673953 [details] [diff] [review]
[checked in] 3db v2
Comment 35 :aceman 2012-10-22 12:24:53 PDT
Ryan, these 2 are the final patches. Bug can be closed if they land successfully, thanks.

Note You need to log in before you can comment on or make changes to this bug.