229705 - CStringArray methods should accept nsACString& instead of nsCString&

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jungshik Shin]

While working on bug 229032, I found that CStringArray::CStringAt only accepts
nsCString& as out parameter while StringArray::StringAt accepts nsAString&. I
don't see any reason for this disparity. Fix coming up.

Comment 1

[Jungshik Shin]

Attachment: patch

.

Comment 2

[Jungshik Shin]

Comment on attachment 138152 [details] [diff] [review]
patch

Asking for r/sr.

Unless there's a reason (unknown to me) to make nsCStringArray and
nsStringArray different, I guess this is all right.

Comment 3

[Alec Flett]

Comment on attachment 138152 [details] [diff] [review]
patch

r=alecf

Comment 4

[Jungshik Shin]

Comment on attachment 138152 [details] [diff] [review]
patch

brendan seems busy. asking bz for sr.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 138152 [details] [diff] [review]
patch

sr=bzbarsky

Comment 6

[Scott MacGregor]

jshin, is there any reason why all of the nsCString& arguments in nsCStringArray
should not be converted over to nsACstring instead of nsCString&?

If you look at the signature for nsStringArray, all the arguments are nsAString&
types. But the signatures look different for the char * version, nsCStringArray
which insists on nsCString&

Comment 7

[Jungshik Shin]

I don't see any reason not to use nsACString for nsCStringArray. I should have
noticed that there are other functions with the same problem as CStringAt had.
(the patch was landed a while ago).

I did a little bit of code archaelogy and it seems like the conversion to
nsAString from nsString was done in two steps. nsString was replaced by
nsAWritableString and nsAReadableString in 3.19. Later in 3.36 they're replaced
by nsAString. In 3.19, CStringArray was not touched probably because it's not
used in DOM. In 3.36, it was not changed because that was a part of tree-wide
change to replace nsA(C)(Writable/Readable)String with nsAString and
CStringArray used (and still uses) nsCString (instead of
nsAC(Writable|Readable)String)


http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/xpcom/ds&command=DIFF_FRAMESET&file=nsVoidArray.cpp&rev2=3.36&rev1=3.35
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsVoidArray.cpp&branch=&root=/cvsroot&subdir=mozilla/xpcom/ds&command=DIFF_FRAMESET&rev1=3.18&rev2=3.19


I'm morphing this bug to deal with other methods of CStringArray. 

Comment 8

[Jungshik Shin]

Attachment: additional patch

I changed other methods to accept nsACString&.

Comment 9

[Scott MacGregor]

Comment on attachment 141435 [details] [diff] [review]
additional patch

I'm happy to put an r=mscott on this patch. I'm assuming because nsACString is
an abstract version of nsCString that this won't break any of the existing
callers.

Comment 10

[Jungshik Shin]

Comment on attachment 141435 [details] [diff] [review]
additional patch

thanks for r. asking jst for sr.

Comment 11

[Alec Flett]

Comment on attachment 141435 [details] [diff] [review]
additional patch

a great change. sr=alecf

Comment 12

[Darin Fisher]

>-      if (nsCRT::strcasecmp((*string).get(),aPossibleString.get())==0)
>+      if (string->EqualsIgnoreCase(PromiseFlatCString(aPossibleString).get()))

this looks suboptimal to me.  how about this instead:

  if (string->Equals(aPossibleString, nsCaseInsensitiveCStringComparator()))

that way you don't force a copy of aPossibleString if it is not null-terminated.

Comment 13

[Jungshik Shin]

alecf, thanks for sr.

darin, we don't have nsCaseInsensitiveCStringComparator although we have
nsCaseInsensitiveStringComparator. :-)

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We do have nsCaseInsensitiveCStringComparitor (it's in nsAString.{h,cpp}).

Also, it might be worth waiting until darin's string branch lands and just using
nsSubstring.

Comment 15

[Jungshik Shin]

Sorry. I should have trusted my memory more than lxr (identifierr search) :-)
 

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 141435 [details] [diff] [review]
additional patch

Also...

- In nsCStringArray::IndexOfIgnoreCase():

     while (ap < end)
     {
       nsCString* string = NS_STATIC_CAST(nsCString*, *ap);
-      if (nsCRT::strcasecmp((*string).get(),aPossibleString.get())==0)
+      if (string->EqualsIgnoreCase(PromiseFlatCString(aPossibleString).get()))

You should at the very least move the PromiseFlatCString().get() out of the
loop since it's constant...

Comment 17

[Alec Flett]

oops, good spot jst.. looks like I gotta brush up a little on my reviewing :)

Comment 18

[Jungshik Shin]

So, which one do you like best?

A.
if (string->Equals(aPossibleString, nsCaseInsensitiveCStringComparator()))

B. 

char *possibleString = aPossibleString.get();
while (.....)
....
if (string->EqualsIgnorecase(possibleString))

or

if (nsCRT::strcasecmp(string->get(), possibleString) == 0)

Comment 19

[Alec Flett]

I prefer the use of strcasecmp() since it avoids the excess vtable calls (i.e.
aPossibleString.get() and the like.. )

unless .get() is no longer virtual with darin's changes?

Comment 20

[Darin Fisher]

(In reply to comment #18)
> So, which one do you like best?
> 
> A.
> if (string->Equals(aPossibleString, nsCaseInsensitiveCStringComparator()))

the advantage of this code is that it will do a length comparison before
comparing the data of the strings.  the length comparison is obviously very
fast.  the only virtual function call (if both strings are implemented using our
internal nsAString implementation) is the comparator call.  but there will be
only one call made to it and that only if the lengths don't match.


> B. 
> 
> char *possibleString = aPossibleString.get();
> while (.....)
> ....
> if (string->EqualsIgnorecase(possibleString))

in this case you end up computing the length of |possibleString| each time
EqualsIgnoreCase is called.  this probably makes this version the worst choice.
 now, technically the string code should be able to make this call equivalent to
the one below.  but, there's also the fact that this method is part of the
deprecated/obsolete/rickg string API.  you aren't supposed to be using it! ;-)


> or
> 
> if (nsCRT::strcasecmp(string->get(), possibleString) == 0)

this version is pretty good in that it minimizes the overhead of the comparator
call in the first version.  however, it doesn't have the length checking
optimization.  i guess you have to decide if the length checking optimization is
worth it to you.  frankly i suspect it is.

.get() is inlined in all cases (now) to a simple member variable accessor
(return mData;).  the only exception is nsXPIDLString::get(), which must do a
little more work in order to support the fact that nsXPIDLString::get()
sometimes returns nsnull.

Comment 21

[Jungshik Shin]

forgot to mark this as fixed. I went with 'A' as darin suggested.