Closed Bug 466622 Opened 11 years ago Closed 11 years ago

Replace nsCStringArray with nsTArray<nsCString>

Categories

(Core :: XPCOM, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: fred.jen, Assigned: fred.jen)

References

Details

Attachments

(2 files, 1 obsolete file)

Same advantages like for the replacement of nsStringArray.

But ParseString has to be moved to nsStringAPI.h

What happens with :
IndexOfIgnoreCase
RemoveCStringIgnoreCase
Assignee: nobody → fred.jen
Attached patch replace nsCStringArray - v1 (obsolete) — Splinter Review
* 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 ?
Attachment #350729 - Flags: review?(roc)
(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!!!
Changed as proposed.
Attachment #350729 - Attachment is obsolete: true
Attachment #350933 - Flags: review?(roc)
Attachment #350729 - Flags: review?(roc)
Put ParseString in nsCRTGlue.h/cpp and it will be available to both internal and external clients.
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 ?
What in nsReadableUtils do you need?
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.
Attached patch updated patchSplinter Review
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.
Attachment #357647 - Flags: review?(benjamin)
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 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.
Attachment #357647 - Flags: review?(benjamin) → review+
OK thanks
Whiteboard: [needs landing]
Pushed http://hg.mozilla.org/mozilla-central/rev/d001c3ce1598 with the tests.
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Fred: thanks for doing this, and the other bugs!!!
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.
Depends on: 474759
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?
No. nsStringEnumerator.h is not part of the glue and cannot be used from frozen-linkage code.
Depends on: 475023
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.)
OS: Linux → All
Version: unspecified → Trunk
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.
The three uses of IndexOf in nsHTMLEditor.cpp look wrong (bug 461047 changed mStyleSheetURLs).
(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
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.
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.
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.
So did the new ParseString remove the useful feature to have several different
delimiters?
cstringarray.ParseString(somestring, "\t\n "); worked in the old code.
You need to log in before you can comment on or make changes to this bug.