Last Comment Bug 802135 - Replace nsresult equality tests with macros
: Replace nsresult equality tests with macros
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: P5 trivial (vote)
: Thunderbird 20.0
Assigned To: :aceman
:
:
Mentors:
Depends on: 801383
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-16 07:09 PDT by :Irving Reid (No longer working on Firefox)
Modified: 2012-12-13 14:44 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
part 1 - News & MIME [checkin: comment 13] (4.24 KB, patch)
2012-11-21 14:37 PST, :aceman
Pidgeot18: review+
Details | Diff | Splinter Review
part 2 - filters [checkin: comment 13] (4.69 KB, patch)
2012-11-21 14:38 PST, :aceman
rkent: review+
Details | Diff | Splinter Review
part 3 - compose [checkin: comment 10] (3.70 KB, patch)
2012-11-21 14:38 PST, :aceman
neil: review+
mkmelin+mozilla: feedback+
Details | Diff | Splinter Review
part 4 - Addressbook [checkin: comment 13] (20.52 KB, patch)
2012-11-21 14:44 PST, :aceman
mconley: review+
Details | Diff | Splinter Review
part 5 - msgdb (32.87 KB, patch)
2012-11-24 14:39 PST, :aceman
irving: review-
Details | Diff | Splinter Review
part 6 - misc [checkin: comment 10] (3.43 KB, patch)
2012-11-24 14:54 PST, :aceman
neil: review+
Details | Diff | Splinter Review
part 5 - msgdb v2 (33.10 KB, patch)
2012-12-05 11:02 PST, :aceman
irving: review+
Details | Diff | Splinter Review
part 5 - msgdb v3 [checkin: comment 20] (33.04 KB, patch)
2012-12-12 11:06 PST, :aceman
acelists: review+
Details | Diff | Splinter Review

Description :Irving Reid (No longer working on Firefox) 2012-10-16 07:09:21 PDT
+++ 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.
Comment 1 :aceman 2012-10-16 08:10:05 PDT
Thanks, I try this.
Comment 2 :aceman 2012-11-21 13:30:32 PST
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 :))
Comment 3 :aceman 2012-11-21 14:37:45 PST
Created attachment 684188 [details] [diff] [review]
part 1 - News & MIME [checkin: comment 13]
Comment 4 :aceman 2012-11-21 14:38:18 PST
Created attachment 684189 [details] [diff] [review]
part 2 - filters [checkin: comment 13]
Comment 5 :aceman 2012-11-21 14:38:58 PST
Created attachment 684190 [details] [diff] [review]
part 3 - compose [checkin: comment 10]
Comment 6 :aceman 2012-11-21 14:44:38 PST
Created attachment 684195 [details] [diff] [review]
part 4 - Addressbook [checkin: comment 13]
Comment 7 Magnus Melin 2012-11-22 12:27:40 PST
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
Comment 8 :aceman 2012-11-24 14:39:16 PST
Created attachment 684891 [details] [diff] [review]
part 5 - msgdb
Comment 9 :aceman 2012-11-24 14:54:01 PST
Created attachment 684895 [details] [diff] [review]
part 6 - misc [checkin: comment 10]
Comment 11 Mike Conley (:mconley) 2012-11-28 08:59:21 PST
Comment on attachment 684195 [details] [diff] [review]
part 4 - Addressbook [checkin: comment 13]

Looks good - thanks for the cleanup, aceman!
Comment 12 Kent James (:rkent) 2012-11-29 08:30:09 PST
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.
Comment 14 :Irving Reid (No longer working on Firefox) 2012-12-04 09:48:49 PST
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)
Comment 15 :aceman 2012-12-04 12:51:00 PST
Sorry, I can't answer most of the questions.
Comment 16 :aceman 2012-12-05 06:59:39 PST
So I'll separate the controversial changes into a new patch that can hang here and the OK stuff can land.
Comment 17 :aceman 2012-12-05 11:02:21 PST
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.
Comment 18 :Irving Reid (No longer working on Firefox) 2012-12-11 13:47:15 PST
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.
Comment 19 :aceman 2012-12-12 11:06:22 PST
Created attachment 691442 [details] [diff] [review]
part 5 - msgdb v3 [checkin: comment 20]
Comment 20 Ryan VanderMeulen [:RyanVM] 2012-12-13 14:43:32 PST
https://hg.mozilla.org/comm-central/rev/0ea057726c2a

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