Closed Bug 466622 Opened 11 years ago Closed 11 years ago
CString Array with ns TArray<ns CString>
Same advantages like for the replacement of nsStringArray. But ParseString has to be moved to nsStringAPI.h What happens with : IndexOfIgnoreCase RemoveCStringIgnoreCase
* 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 ?
(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.
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.
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+
Whiteboard: [needs landing]
Pushed http://hg.mozilla.org/mozilla-central/rev/d001c3ce1598 with the tests.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Fred: thanks for doing this, and the other bugs!!!
Also checked in bustage fix http://hg.mozilla.org/mozilla-central/rev/fb898c2e6e70
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 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.
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.