The default bug view has changed. See this FAQ.

Replace nsresult equality tests with macros

RESOLVED FIXED in Thunderbird 20.0

Status

MailNews Core
Backend
P5
trivial
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Irving, Assigned: aceman)

Tracking

Trunk
Thunderbird 20.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 2 obsolete attachments)

+++ This bug was initially created as a clone of Bug #801383 +++

After bug 779473 and bug 801383, there are many places in comm-central where we do equality and inequality tests for nsresult enum values. We should sweep through c-c and check all the places where we have "foo == NS_OK" and "foo != NS_OK" and replace them with NS_SUCCEEDED(foo) or NS_FAILED(foo), after checking to make sure the logic doesn't depend on the very few cases of special success values for nsresult.


grep -l -r '[=!]= *NS_OK' mailnews

will give a list of affected filenames on appropriately unix-like systems; all the comm-central occurrences I could find are in mailnews.
(Assignee)

Comment 1

5 years ago
Thanks, I try this.
Assignee: nobody → acelists
(Assignee)

Comment 2

4 years ago
This should also be a tiny perf win as I think the NS_ macros contain "likely"/"unlikely" compiler hints.

For the record these are the files affected according to irving's grep command:
mailnews/mime/cthandlers/pgpmime/nsPgpMimeProxy.cpp
mailnews/news/src/nsNNTPProtocol.cpp
mailnews/news/src/nsNNTPNewsgroupList.cpp
mailnews/addrbook/src/nsAddrDatabase.cpp
mailnews/compose/src/nsMsgSendReport.cpp
mailnews/compose/src/nsMsgSend.cpp
mailnews/compose/src/nsSmtpProtocol.cpp
mailnews/db/msgdb/src/nsMsgHdr.cpp
mailnews/db/msgdb/src/nsDBFolderInfo.cpp
mailnews/db/msgdb/src/nsMsgDatabase.cpp
mailnews/db/msgdb/src/nsMailDatabase.cpp
mailnews/imap/src/nsImapProtocol.cpp
mailnews/local/src/nsParseMailbox.cpp
mailnews/base/search/src/nsMsgSearchTerm.cpp
mailnews/base/search/src/nsMsgFilterList.cpp
mailnews/base/util/nsMsgLineBuffer.cpp
mailnews/base/util/nsMsgDBFolder.cpp

All are in mailnews. (There are tons of others in /mozilla :))
(Assignee)

Comment 3

4 years ago
Created attachment 684188 [details] [diff] [review]
part 1 - News & MIME [checkin: comment 13]
Attachment #684188 - Flags: review?(Pidgeot18)
(Assignee)

Comment 4

4 years ago
Created attachment 684189 [details] [diff] [review]
part 2 - filters [checkin: comment 13]
Attachment #684189 - Flags: review?(kent)
(Assignee)

Comment 5

4 years ago
Created attachment 684190 [details] [diff] [review]
part 3 - compose [checkin: comment 10]
Attachment #684190 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 6

4 years ago
Created attachment 684195 [details] [diff] [review]
part 4 - Addressbook [checkin: comment 13]
Attachment #684195 - Flags: review?(mconley)
(Assignee)

Updated

4 years ago
Severity: normal → trivial
Status: NEW → ASSIGNED
Priority: -- → P5

Comment 7

4 years ago
Comment on attachment 684190 [details] [diff] [review]
part 3 - compose [checkin: comment 10]

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

I'm not a mailnews reviewer, but looks ok to me
Attachment #684190 - Flags: review?(mkmelin+mozilla) → feedback+
(Assignee)

Updated

4 years ago
Attachment #684190 - Flags: review?(neil)

Updated

4 years ago
Attachment #684190 - Flags: review?(neil) → review+
(Assignee)

Updated

4 years ago
Attachment #684190 - Attachment description: part 3 - compose → part 3 - compose [ready for checkin]
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Whiteboard: [please leave open after checkin]
(Assignee)

Comment 8

4 years ago
Created attachment 684891 [details] [diff] [review]
part 5 - msgdb
Attachment #684891 - Flags: review?(irving)
(Assignee)

Comment 9

4 years ago
Created attachment 684895 [details] [diff] [review]
part 6 - misc [checkin: comment 10]
Attachment #684895 - Flags: review?(neil)

Updated

4 years ago
Attachment #684895 - Flags: review?(neil) → review+
(Assignee)

Updated

4 years ago
Attachment #684895 - Attachment description: part 6 - misc → part 6 - misc [ready for checkin]
https://hg.mozilla.org/comm-central/rev/31936f6405bc
https://hg.mozilla.org/comm-central/rev/6d781d7536e0
Keywords: checkin-needed
Attachment #684190 - Attachment description: part 3 - compose [ready for checkin] → part 3 - compose [checkin: comment 10]
Attachment #684895 - Attachment description: part 6 - misc [ready for checkin] → part 6 - misc [checkin: comment 10]
Attachment #684188 - Flags: review?(Pidgeot18) → review+
Comment on attachment 684195 [details] [diff] [review]
part 4 - Addressbook [checkin: comment 13]

Looks good - thanks for the cleanup, aceman!
Attachment #684195 - Flags: review?(mconley) → review+
(Assignee)

Updated

4 years ago
Attachment #684195 - Attachment description: part 4 - Addressbook → part 4 - Addressbook [ready for checkin]
(Assignee)

Updated

4 years ago
Attachment #684188 - Attachment description: part 1 - News & MIME → part 1 - News & MIME [ready for checkin]
(Assignee)

Updated

4 years ago
Keywords: checkin-needed

Comment 12

4 years ago
Comment on attachment 684189 [details] [diff] [review]
part 2 - filters [checkin: comment 13]

Sorry for the slow review, both Thanksgiving and compile issues slowed me down.
Attachment #684189 - Flags: review?(kent) → review+
https://hg.mozilla.org/comm-central/rev/3279270bcb0d
https://hg.mozilla.org/comm-central/rev/9dbbeb38c567
https://hg.mozilla.org/comm-central/rev/79689e5b9cc0
Keywords: checkin-needed
Attachment #684188 - Attachment description: part 1 - News & MIME [ready for checkin] → part 1 - News & MIME [checkin: comment 13]
Attachment #684195 - Attachment description: part 4 - Addressbook [ready for checkin] → part 4 - Addressbook [checkin: comment 13]
Attachment #684189 - Attachment description: part 2 - filters → part 2 - filters [checkin: comment 13]
Comment on attachment 684891 [details] [diff] [review]
part 5 - msgdb

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

Sorry for the slow review. Just a few questions and comments; most of it looks great.

::: mailnews/db/msgdb/src/nsMailDatabase.cpp
@@ +244,5 @@
>            }
>          }
>        }
>      }
> +    rv = (NS_SUCCEEDED(err)) ? NS_OK : NS_ERROR_FAILURE;

Not sure why we're discarding the value of err here - would it cause a problem to replace this with "rv = err;" ?

@@ +254,5 @@
>  }
>  
>  NS_IMETHODIMP nsMailDatabase::ListAllOfflineDeletes(nsTArray<nsMsgKey> *offlineDeletes)
>  {
> +  NS_ENSURE_ARG_POINTER(offlineDeletes);

This will print a warning in debug builds, which we don't want to do if it's normal to call this method with a null pointer some of the time. It probably isn't normal in this case, since the original code returned NS_ERROR_NULL_POINTER, but it's worth making sure all the NS_ENSURE_ARG_POINTER changes you make aren't adding false warnings to our debug output.

@@ +293,5 @@
>          }
>          offlineOpRow->Release();
>        }
>      }
> +    rv = (NS_SUCCEEDED(err)) ? NS_OK : NS_ERROR_FAILURE;

Again, would "rv = err;" cause problems here?

@@ +401,4 @@
>    mdbOid outOid;
> +  nsMsgKey key = 0;
> +  // Is the key variable unused?
> +  if (NS_SUCCEEDED(offlineOpRow->GetOid(mDB->GetEnv(), &outOid)))

Good question. Looks like it has been unused since at least the switch to mercurial (http://hg.mozilla.org/comm-central/annotate/e4f4569d451a/mailnews/db/msgdb/src/nsMailDatabase.cpp), you could dig back into the old CVS history to see if it was used some time in the past.

Before removing it, make sure the offlineOpRow->GetOid(mDB->GetEnv()...) doesn't have important side effects. If there are no side effects, outOid is also unused.

Might be worth taking this out into its own bug if you're going to follow up.

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +1605,5 @@
>    tableOID.mOid_Scope = scopeToken;
>    tableOID.mOid_Id = 1;
>  
>    nsresult rv = m_mdbStore->GetTable(GetEnv(), &tableOID, table);
> +  NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);

Again, not sure why we're replacing the actual error code in rv with NS_ERROR_FAILURE here - unless the caller specifically depends on getting NS_ERROR_FAILURE here I'd use NS_ENSURE_SUCCESS(rv, rv)

The only thing I can think of is if we're meant to turn non-NS_OK success codes into failures; can m_mdbStore->GetTable() return any of the codes found by http://mxr.mozilla.org/comm-central/ident?i=NS_MSG_GENERATE_SUCCESS ?

@@ +2828,1 @@
>        key = outOid.mOid_Id;

This looks suspiciously like the code at http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMailDatabase.cpp#405 (where you commented that 'key' is unused) - i wonder if there was cut&paste...

@@ +3279,2 @@
>      if (NS_FAILED(rv))
>        return rv;

Check the revision history to see how it got there; remove it unless the revision history shows it's there because of some other mistake (in which case, file a bug for the other mistake)
Attachment #684891 - Flags: review?(irving) → review-
(Assignee)

Comment 15

4 years ago
Sorry, I can't answer most of the questions.
(Assignee)

Comment 16

4 years ago
So I'll separate the controversial changes into a new patch that can hang here and the OK stuff can land.
(Assignee)

Comment 17

4 years ago
Created attachment 688873 [details] [diff] [review]
part 5 - msgdb v2

Actually there is nothing left to separate. The code could go in even without solving the questions.

(In reply to Irving Reid (:irving) from comment #14)
> ::: mailnews/db/msgdb/src/nsMailDatabase.cpp
> @@ +244,5 @@
> >            }
> >          }
> >        }
> >      }
> > +    rv = (NS_SUCCEEDED(err)) ? NS_OK : NS_ERROR_FAILURE;
> 
> Not sure why we're discarding the value of err here - would it cause a
> problem to replace this with "rv = err;" ?

Added comment. I do not change the logic so my change is safe.

> @@ +254,5 @@
> >  }
> >  
> >  NS_IMETHODIMP nsMailDatabase::ListAllOfflineDeletes(nsTArray<nsMsgKey> *offlineDeletes)
> >  {
> > +  NS_ENSURE_ARG_POINTER(offlineDeletes);
> 
> This will print a warning in debug builds, which we don't want to do if it's
> normal to call this method with a null pointer some of the time. It probably
> isn't normal in this case, since the original code returned
> NS_ERROR_NULL_POINTER, but it's worth making sure all the
> NS_ENSURE_ARG_POINTER changes you make aren't adding false warnings to our
> debug output.

The ListAllOfflineOpIds() function above looks very similar to this ListAllOfflineDeletes() function so I just make this NS_ENSURE the same. Hopefully their callers also are similar. If it is a problem, I can just drop this, it does not relate to this bug.

> @@ +293,5 @@
> >          }
> >          offlineOpRow->Release();
> >        }
> >      }
> > +    rv = (NS_SUCCEEDED(err)) ? NS_OK : NS_ERROR_FAILURE;
> 
> Again, would "rv = err;" cause problems here?
Added comment. I do not change the logic so my change is safe.

> 
> @@ +401,4 @@
> >    mdbOid outOid;
> > +  nsMsgKey key = 0;
> > +  // Is the key variable unused?
> > +  if (NS_SUCCEEDED(offlineOpRow->GetOid(mDB->GetEnv(), &outOid)))
> 
> Good question. Looks like it has been unused since at least the switch to
> mercurial
> (http://hg.mozilla.org/comm-central/annotate/e4f4569d451a/mailnews/db/msgdb/
> src/nsMailDatabase.cpp), you could dig back into the old CVS history to see
> if it was used some time in the past.
> 
> Before removing it, make sure the offlineOpRow->GetOid(mDB->GetEnv()...)
> doesn't have important side effects. If there are no side effects, outOid is
> also unused.
> 
> Might be worth taking this out into its own bug if you're going to follow up.
OK, I've just left the new comment in without removing the variable. It is not related to this bug.

> ::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
> @@ +1605,5 @@
> >    tableOID.mOid_Scope = scopeToken;
> >    tableOID.mOid_Id = 1;
> >  
> >    nsresult rv = m_mdbStore->GetTable(GetEnv(), &tableOID, table);
> > +  NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
> 
> Again, not sure why we're replacing the actual error code in rv with
> NS_ERROR_FAILURE here - unless the caller specifically depends on getting
> NS_ERROR_FAILURE here I'd use NS_ENSURE_SUCCESS(rv, rv)
> 
> The only thing I can think of is if we're meant to turn non-NS_OK success
> codes into failures; can m_mdbStore->GetTable() return any of the codes
> found by http://mxr.mozilla.org/comm-central/ident?i=NS_MSG_GENERATE_SUCCESS
> ?

No idea. But this specific return value was there before, I do not change the logic, so it should be safe.

> @@ +3279,2 @@
> >      if (NS_FAILED(rv))
> >        return rv;
> 
> Check the revision history to see how it got there; remove it unless the
> revision history shows it's there because of some other mistake (in which
> case, file a bug for the other mistake)

OK, just added a comment.
Attachment #684891 - Attachment is obsolete: true
Attachment #688873 - Flags: review?(irving)
Comment on attachment 688873 [details] [diff] [review]
part 5 - msgdb v2

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

r+ with the bit below removed.

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +3281,2 @@
>      if (NS_FAILED(rv))
>        return rv;

OK, I looked it up - this code was added as a big chunk in 1999, in this change: http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/mailnews/db/msgdb/src&command=DIFF_FRAMESET&file=nsMsgDatabase.cpp&rev2=1.64&rev1=1.63

I think it's junk, just remove it.
Attachment #688873 - Flags: review?(irving) → review+
(Assignee)

Comment 19

4 years ago
Created attachment 691442 [details] [diff] [review]
part 5 - msgdb v3 [checkin: comment 20]
Attachment #688873 - Attachment is obsolete: true
Attachment #691442 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Whiteboard: [please leave open after checkin]
https://hg.mozilla.org/comm-central/rev/0ea057726c2a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 20.0
Attachment #691442 - Attachment description: part 5 - msgdb v3 [ready for checkin] → part 5 - msgdb v3 [checkin: comment 20]
You need to log in before you can comment on or make changes to this bug.