The default bug view has changed. See this FAQ.

fix some "warning: variable 'rv' set but not used [-Wunused-but-set-variable]" gcc warnings in /mailnews

RESOLVED FIXED in Thunderbird 19.0

Status

MailNews Core
Backend
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 19.0
x86
Linux
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 7 obsolete attachments)

(Assignee)

Description

5 years ago
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]
(Assignee)

Comment 1

5 years ago
I'll make several patches so that each module owner can review his part.
(Assignee)

Comment 2

5 years ago
Created attachment 666721 [details] [diff] [review]
1base
Attachment #666721 - Flags: review?(neil)
(Assignee)

Comment 3

5 years ago
Created attachment 666723 [details] [diff] [review]
2compose
Attachment #666723 - Flags: review?(neil)
(Assignee)

Comment 4

5 years ago
Created attachment 666725 [details] [diff] [review]
3db
Attachment #666725 - Flags: review?(mozilla)
(Assignee)

Comment 5

5 years ago
Created attachment 666727 [details] [diff] [review]
[checked in] 4smime
Attachment #666727 - Flags: review?(kaie)
(Assignee)

Comment 6

5 years ago
Created attachment 666728 [details] [diff] [review]
5import
Attachment #666728 - Flags: review?(mbanner)
(Assignee)

Comment 7

5 years ago
Created attachment 666729 [details] [diff] [review]
6news
Attachment #666729 - Flags: review?(Pidgeot18)
(Assignee)

Comment 8

5 years ago
Created attachment 666730 [details] [diff] [review]
[checked in] 7imap
Attachment #666730 - Flags: review?(mozilla)
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).
Attachment #666729 - Flags: review?(Pidgeot18) → review+
(Assignee)

Comment 10

5 years ago
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' :)
(Assignee)

Comment 11

5 years ago
Created attachment 667049 [details] [diff] [review]
[checked in] 6news v2
Attachment #666729 - Attachment is obsolete: true
Attachment #667049 - Flags: review+
(Assignee)

Comment 12

5 years ago
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.
Keywords: checkin-needed
Whiteboard: [leave bug open after checkin]
Attachment #666727 - Flags: review?(kaie) → review?(irving)
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.
Attachment #666727 - Flags: review?(irving) → review+
(Assignee)

Updated

5 years ago
Attachment #666727 - Attachment description: 4smime → 4smime [ready for check-in, r+ in comment 13, please fix reviewer]
Comment on attachment 666727 [details] [diff] [review]
[checked in] 4smime

https://hg.mozilla.org/comm-central/rev/005b8cd8a647
Attachment #666727 - Attachment description: 4smime [ready for check-in, r+ in comment 13, please fix reviewer] → [checked in] 4smime
Comment on attachment 667049 [details] [diff] [review]
[checked in] 6news v2

https://hg.mozilla.org/comm-central/rev/611934f581bf
Attachment #667049 - Attachment description: 6news v2 [ready for check-in, r+ in comment 9] → [checked in] 6news v2
Keywords: checkin-needed
Whiteboard: [leave bug open after checkin] → [leave open]
(Assignee)

Updated

5 years ago
Blocks: 187528
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.
Attachment #666728 - Flags: review?(mbanner) → review-
(Assignee)

Comment 17

5 years ago
Created attachment 667571 [details] [diff] [review]
5import v2
Attachment #666728 - Attachment is obsolete: true
Attachment #667571 - Flags: review?(mbanner)
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.
Attachment #666721 - Flags: review?(neil) → review+
(Assignee)

Comment 19

5 years ago
Created attachment 669201 [details] [diff] [review]
[checked in ] 1base v2
Attachment #666721 - Attachment is obsolete: true
Attachment #669201 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
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.
Keywords: checkin-needed
Comment on attachment 669201 [details] [diff] [review]
[checked in ] 1base v2

https://hg.mozilla.org/comm-central/rev/03fd0a59e409
Attachment #669201 - Attachment description: 1base v2 [ready for checkin, r+ in comment 18] → [checked in ] 1base v2
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.
Attachment #667571 - Flags: review?(mbanner) → review+
(Assignee)

Comment 23

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

Comment 25

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

Comment 26

5 years ago
Created attachment 669648 [details] [diff] [review]
[checked in] 2compose v2
Attachment #666723 - Attachment is obsolete: true
Attachment #666723 - Flags: review?(neil)
Attachment #669648 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #669648 - Flags: review+ → review?(neil)
(Assignee)

Comment 27

5 years ago
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.
Attachment #667571 - Attachment is obsolete: true
Attachment #669649 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Keywords: checkin-needed
Comment on attachment 669649 [details] [diff] [review]
5import v3

https://hg.mozilla.org/comm-central/rev/de7b9f15cd2e
Attachment #669649 - Attachment description: 5import v3 [ready for checkin, r+ in comment 22] → [checked in] 5import v3
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
Attachment #669649 - Attachment description: [checked in] 5import v3 → 5import v3
Attachment #669649 - Attachment is obsolete: true
(Assignee)

Comment 30

5 years ago
Created attachment 670035 [details] [diff] [review]
[checked in] 5import v4

Sorry about that, try this.
Attachment #670035 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed

Updated

5 years ago
Attachment #669648 - Flags: review?(neil) → review+
(Assignee)

Updated

5 years ago
Attachment #669648 - Attachment description: 2compose v2 → 2compose v2 [ready for checkin, r+ in comment 30.5]
Keywords: checkin-needed
Comment on attachment 669648 [details] [diff] [review]
[checked in] 2compose v2

https://hg.mozilla.org/comm-central/rev/2bfa5fb694bf
Attachment #669648 - Attachment description: 2compose v2 [ready for checkin, r+ in comment 30.5] → [checked in] 2compose v2
Comment on attachment 670035 [details] [diff] [review]
[checked in] 5import v4

https://hg.mozilla.org/comm-central/rev/9c4f3220eff9
Attachment #670035 - Attachment description: 5import v4 [ready for checkin, r+ in comment 22] → [checked in] 5import v4
(Assignee)

Updated

5 years ago
Attachment #666725 - Flags: review?(irving)
(Assignee)

Updated

5 years ago
Attachment #666730 - Flags: review?(mbanner)
Attachment #666730 - Flags: review?(mozilla)
Attachment #666730 - Flags: review?(mbanner)
Attachment #666730 - Flags: review?(irving)
Attachment #666730 - Flags: review?(irving) → review+
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
Attachment #666725 - Flags: review?(irving) → review-
(Assignee)

Updated

5 years ago
Attachment #666730 - Attachment description: 7imap → 7imap [ready for checkin, r+ before comment 33]
(Assignee)

Comment 34

5 years ago
Created attachment 673953 [details] [diff] [review]
[checked in] 3db v2
Attachment #666725 - Attachment is obsolete: true
Attachment #666725 - Flags: review?(mozilla)
Attachment #673953 - Flags: review?(irving)
Attachment #673953 - Flags: review?(irving) → review+
(Assignee)

Updated

5 years ago
Attachment #673953 - Attachment description: 3db v2 → 3db v2 [ready for checkin, r+ after comment 34]
(Assignee)

Comment 35

5 years ago
Ryan, these 2 are the final patches. Bug can be closed if they land successfully, thanks.
Keywords: checkin-needed
Whiteboard: [leave open]
https://hg.mozilla.org/comm-central/rev/d2466c3e263f
https://hg.mozilla.org/comm-central/rev/9ebeb895fc29
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 19.0
Attachment #666730 - Attachment description: 7imap [ready for checkin, r+ before comment 33] → [checked in] 7imap
Attachment #673953 - Attachment description: 3db v2 [ready for checkin, r+ after comment 34] → [checked in] 3db v2
You need to log in before you can comment on or make changes to this bug.