466622 - Replace nsCStringArray with nsTArray<nsCString>

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Fred Jendrzejewski]

Same advantages like for the replacement of nsStringArray.

But ParseString has to be moved to nsStringAPI.h

What happens with :
IndexOfIgnoreCase
RemoveCStringIgnoreCase

Comment 1

[Fred Jendrzejewski]

Attachment: replace nsCStringArray - v1

* The patch applies on top of the patch from bug #461047, but I can change this if wished.
* I moved ParseString into the nsReadableUtils.h but I didn't really find out where to add anything in nsStringAPI.h
* I replaced the IndexOfIgnoreCase by adding a new comparator class in the concerning file, maybe a standard nsIgnoreCaseComparator would be better ?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #1)
> * I moved ParseString into the nsReadableUtils.h but I didn't really find out
> where to add anything in nsStringAPI.h

You can just add the function there. However, I'm not sure if that means we have to duplicate the function, since nsStringAPI can't be used from internal-linkage code. Benjamin?

> * I replaced the IndexOfIgnoreCase by adding a new comparator class in the
> concerning file, maybe a standard nsIgnoreCaseComparator would be better ?

I think this is OK for now.

+    rv = (array->AppendElement(alias) != nsnull);

Unnecessary parens. But more importantly, you're assigning a boolean to an nsresult, which isn't right. You want something like
  rv = array->AppendElement(alias) ? NS_OK : NS_ERROR_OUT_OF_MEMORY;

   // Parses a given string using the delimiter passed in. If the array
   // already has some elements, items parsed from string will be appended 
   // to array. For example, array.ParseString("a,b,c", ","); will add strings
   // "a", "b" and "c" to the array. Parsing process has the same tokenizing 
   // behavior as strtok().  
-  PRBool ParseString(const char* string, const char* delimiter);

You should remove the comment here too.

+ParseString (const nsACString& aSource, const nsACString& aDelimiter, 
+              nsTArray<nsCString>& aArray)

Unnecessary space after ParseString. Also, fix indent of the second line.

Really nice work, thank you!!!

Comment 3

[Fred Jendrzejewski]

Attachment: cleaned as proposed

Changed as proposed.

Comment 4

[Benjamin Smedberg]

Put ParseString in nsCRTGlue.h/cpp and it will be available to both internal and external clients.

Comment 5

[Fred Jendrzejewski]

Benjamin : I need for my ParseString nsReadableUtils, but including this in nsCRT doesn't work. Can I acutually use nsTArray<nsCString> and nsCString manipulating functions in nsCRTGlue ?

Comment 6

[Benjamin Smedberg]

What in nsReadableUtils do you need?

Comment 7

[Fred Jendrzejewski]

If FindInReadable isn't available, I have to move back to the old version of the code, based on chars. I'd like to avoid this, because it created a local copies in each step of the parsing process and now we got rid of them. I'll  search a little bit and hope to find maybe another option that works on strings without local copies.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated patch

Updated to latest trunk. I added a ParseString method to nsStringAPI.h that's the same signature as the nsReadableUtils version, but implemented slightly differently. I also changed its signature to take a single separator character since all but one caller needs that and that caller (nsSaveAsCharset) is easy to fix --- requested review on that change.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh, Fred's version of ParseString actually had a bug, it seemed to require a trailing separator or the last element wouldn't be added to the array. I fixed that and also have it skip empty elements like the old ParseString did.

Comment 10

[Benjamin Smedberg]

Comment on attachment 357647 [details] [diff] [review]
updated patch

This is fine, but it would be really nice to have tests for both versions of ParseString.

The tests should be added to
http://mxr.mozilla.org/mozilla/source/xpcom/tests/external/TestMinStringAPI.cpp for the external API and http://mxr.mozilla.org/mozilla/source/xpcom/tests/TestStrings.cpp for the internal API.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK thanks

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed http://hg.mozilla.org/mozilla-central/rev/d001c3ce1598 with the tests.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Fred: thanks for doing this, and the other bugs!!!

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Also checked in bustage fix http://hg.mozilla.org/mozilla-central/rev/fb898c2e6e70

Comment 15

[Fred Jendrzejewski]

I liked to do this work as I learned a lot, I hope that I'll find some quiet moments one day to work here again.

Comment 16

[[e-mail]]

Comment on attachment 357647 [details] [diff] [review]
updated patch

>diff --git a/xpcom/ds/nsStringEnumerator.h b/xpcom/ds/nsStringEnumerator.h
>--- a/xpcom/ds/nsStringEnumerator.h
>+++ b/xpcom/ds/nsStringEnumerator.h
>@@ -32,17 +32,17 @@
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsIStringEnumerator.h"
>-#include "nsVoidArray.h"
>+#include "nsString.h"
Should this not have been nsStringGlue.h?

Comment 17

[Benjamin Smedberg]

No. nsStringEnumerator.h is not part of the glue and cannot be used from frozen-linkage code.

Comment 18

[Daniel Holbert [:dholbert]]

A semi-scary heads-up on this bug -- nsCStringArray::IndexOf returns -1 (as a PRInt32) on failure, but nsTArray's return-value is **unsigned**, so it returns +infinity on failure.

So, all conditionals like this:
   if (myStringArray.IndexOf(testString) < 0)
are now broken, for any variable that changed from nsCStringArray to nsTArray in this bug's patch.

We should find and fix all situations like the above -- otherwise, we're setting ourselves up for all sorts of unexpected behavior.

(I filed bug 475078 on a hang caused by the above issue.)

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The patch converted some IndexOfs to Contains. I guess we missed at least one. I did a crude audit by searching for all lines that matched "IndexOf" and (">= *0" or "< *0"); it found the one in bug 475078, but no others.

Comment 20

[Aiko]

The three uses of IndexOf in nsHTMLEditor.cpp look wrong (bug 461047 changed mStyleSheetURLs).

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to comment #20)
> The three uses of IndexOf in nsHTMLEditor.cpp look wrong
Agreed -- I'm pasting them here for reference, just since that file takes a little while to load on MXR.  All three of the uses probably trigger crashes now, I'd imagine.

3753   foundIndex = mStyleSheetURLs.IndexOf(aURL);
3754   if (foundIndex < 0)
3755     return NS_OK; //No sheet -- don't fail!
3756 
3757   *aStyleSheet = mStyleSheets[foundIndex];

3771   PRInt32 foundIndex = mStyleSheets.IndexOf(aStyleSheet);
3772 
3773   // Don't fail if we don't find it in our list
3774   if (foundIndex == -1)
3775     return NS_OK;
3776 
3777   // Found it in the list!
3778   aURL = mStyleSheetURLs[foundIndex];

3771   PRInt32 foundIndex = mStyleSheets.IndexOf(aStyleSheet);
3772 
3773   // Don't fail if we don't find it in our list
3774   if (foundIndex == -1)
3775     return NS_OK;
3776 
3777   // Found it in the list!
3778   aURL = mStyleSheetURLs[foundIndex];

http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditor.cpp

Comment 22

[Daniel Holbert [:dholbert]]

oops, sorry -- I missed the fact that mStyleSheetURLs was changed to use nsTArray in bug 461047, not in this bug.  Let's move the conversation regarding nsHTMLEditor.cpp to that bug.

So, as far as we know, the patch on this bug here didn't cause any other regressions, per comment 19.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually I don't think those will cause bugs, since foundIndex is declared PRInt32 so assigning NoIndex to it will set it to -1. But thanks for fixing them up anyway.

Comment 24

[Ben Bucksch (:BenB)]

Would you please also adapt https://developer.mozilla.org/en/XPCOM_array_guide , and put warning signs?
The guide suggests nsCStringArray, and that works fine in mozilla 1.9.1 that Thunderbird 3 uses, but when I checked it, the Thunderbird-on-mozilla-central tinderbox caught fire, and I spent the whole night (8 AM local time now) finding out what's going on and compiling trees.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

So did the new ParseString remove the useful feature to have several different
delimiters?
cstringarray.ParseString(somestring, "\t\n "); worked in the old code.