Replace nsCStringArray in news with nsTArray counterparts

RESOLVED FIXED in Thunderbird 10.0

Status

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Trunk
Thunderbird 10.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Posted patch The fix (obsolete) — Splinter Review
nsCStringArray is getting old and obsolete, this helps removing it get a bit further along.
Attachment #559737 - Flags: review?(dbienvenu)
I'm still working on fixing the other one, but this one should work fine.
Comment on attachment 559737 [details] [diff] [review]
The fix

r=me, but no need for braces here:

+  for (PRUint32 i = 0; i < length; ++i)
+  {
+    SetAsSubscribed(mSubscribedNewsgroups[i]);
+  }
Attachment #559737 - Flags: review?(dbienvenu) → review+
Posted patch The fix v2Splinter Review
Ok, this is updated for the PRBool changes, and I also made the AppendIfSearchMatch inline to get rid of the extra function.
Attachment #559737 - Attachment is obsolete: true
Attachment #564792 - Flags: review?(dbienvenu)
Comment on attachment 564792 [details] [diff] [review]
The fix v2

+    hostInfoStream->Write(mGroupsOnServer[i].get(), mGroupsOnServer[i].Length(),
+                  &bytesWritten);

bytesWritten could be indented more to line up with mGroupsOnServer.
Attachment #564792 - Flags: review?(dbienvenu) → review+
Checked in: http://hg.mozilla.org/comm-central/rev/7767f7386c9d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 10.0
Depends on: 693365
Comment on attachment 564792 [details] [diff] [review]
The fix v2

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

::: mailnews/news/src/nsNntpIncomingServer.cpp
@@ -1916,4 +1880,4 @@
> >      if (colID[0] == 'n') {
> >        nsCAutoString str;
> >        if (mSearchResultSortDescending)
> > -        row = mSubscribeSearchResult.Count() + ~row;
> > +        row = mSubscribeSearchResult.Length() + ~row;

nsCStringArray::Count() returns a PRInt32. nsTArray::Length() returns a PRUint32. This, I suspect, is what caused bug 693365. (~row = -row - 1 = -(row + 1)). I think this (and a few other places we play the ~row trick) should be fixed to use - (row + 1), since it's clearer and any decent optimizer could make the two forms equivalent. I just confirmed that gcc -O3 is smart enough to do this.
Comment on attachment 564792 [details] [diff] [review]
The fix v2

>       nsCAutoString str;
>       if (mSearchResultSortDescending)
>-        row = mSubscribeSearchResult.Count() + ~row;
>-      mSubscribeSearchResult.CStringAt(row, str);
>+        row = mSubscribeSearchResult.Length() + ~row;
>+      mSubscribeSearchResult.SafeElementAt(row, str);
>       // some servers have newsgroup names that are non ASCII.  we store
>       // those as escaped. unescape here so the UI is consistent
>       rv = NS_MsgDecodeUnescapeURLPath(str, _retval);

So near, and yet so far... +~row compiles to the same code in signed and unsigned arithmetic, although that doesn't make it readable. The actual error is that the weird CStringAt function was not translated correctly, it should be something like
if (mSearchResultSortDescending)
  row = mSubscribeSearchResult.Length() - 1 - row;
rv = NS_MsgDecodeUnescapeURLPath(mSubscribeSearchResult.ElementAt(row), _retval);
You need to log in before you can comment on or make changes to this bug.