173601 - nsIStringEnumerator

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Darin Fisher]

it would be really great if we had specialized interfaces for arrays of AString
and ACString to avoid nsISupportsAString and nsISupportsACString.  something
like this:

interface nsIStringArray : nsISupports
{
  readonly attribute unsigned long length;
  AString getElementAt(in unsigned long index); 
  unsigned long indexOf(in unsigned long startIndex, in ACString element);
  nsIStringEnumerator enumerate();
};

interface nsIStringEnumerator : nsISupports
{
  boolean hasMore();
  AString getNext();
};

ditto for ACString.  i think it is worthwhile to special case strings since
string arrays are so common.  i'm not sure that nsIStringEnumerator is the
correct name... wouldn't want to confuse it with iterating over a string, but
nsIStringArrayEnumerator seems wrong since the enumeration need not be of an array.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

your suggested interface has apparently no way to append a string to the array

Comment 2

[Alec Flett]

beisi: http://searchcrm.techtarget.com/sDefinition/0,,sid11_gci840566,00.html

Comment 3

[Darin Fisher]

yes, that's the point.  i should have clarified that this is meant to parallel
nsIArray.  hence, there would be a nsIMutableStringArray, which would mimic the
functionality of nsIMutableArray.

Comment 4

[Simon Fraser [no longer active]]

nsICommandParams needs an enumerator over string values. This an API we want to
freeze soon.

Comment 5

[Juan José Mata]

Nominatig topembed since it blocks bug 191523 which I just nominated as well.

Comment 6

[Michael Buckland]

Discussed in edt bug triage.  Plussing because blocks 191523.

Comment 7

[Alec Flett]

I've got a working version of this. I had to make both a copying and a
non-copying version of the enumerator... The non-copying version was darin's
suggestion, and the copying one is for bug 194289

I'm attaching a straw man patch in a bit. I still need to merge the
unicode/non-unicode versions of the copying enumerator together.. they share
enough code that it might be worth it.

Comment 8

[Alec Flett]

Attachment: straw man implementation

here's the implementation...

Comment 9

[Alec Flett]

Attachment: straw man implementation

ack, lets try that again.. this one actually includes the interface... note
that I provided a base interface and then two subinterfaces that deal with each
kind of string... 

also note that I also internally implement nsISimpleEnumerator and give back
the appropriate primitive type. This will allow us to reuse this in classes
like nsICategoryManager, which is frozen and gives back an nsISimpleEnumerator
class. Consumers of nsICategoryManager can QI() to nsIStringEnumerator or
nsIUTF8StringEnumerator if they are so inclined.
http://lxr.mozilla.org/seamonkey/source/xpcom/components/nsICategoryManager.idl#85


(and jeez, we need to update the docs to nsICategoryManager to explain what
crazyness is going on there)

Comment 10

[Alec Flett]

Attachment: first candidate

ok, I think this is ready to go. a few updates since the last patch:
- used some template methods to simplify the constructors tremendously
- combined the 2 CopyingEnumerators into a single enumerator that handles both
unicode and utf8 strings.
- implemented nsISimpleEnumerator on the copying enumerator too.

Comment 11

[Alec Flett]

Comment on attachment 115428 [details] [diff] [review]
first candidate

doug/darin, yet another set of patches for you to review.. hopefully the patch
should speak for itself. Its implementation of a scriptable enumerator for
strings.

I commented pretty heavily throughout the code, so it should be pretty straight
forward.

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 115428 [details] [diff] [review]
first candidate

fix the license date.
add some javadocs to the interfaces so that when we go to freeze them, it is
less painful. 
Do we really have to provide NS_New functions?

OMG - a union.	someone is thinking about footprint. :-)

+    // XXX this needs CopyUCS2toUTF8(aResult,
+    // *mArray->StringAt(mIndex++));

what about a threadsafe version of this impl.  Do we care?

we should talk to wtc and the intl folks about adding support for a
strlen(PRUnichar)

+// so that the template routine will work for strlen()
+static PRUint32 strlen(const PRUnichar* aSrc)
+{
+    return nsCRT::strlen(aSrc);
+}

all for now...

Comment 13

[Darin Fisher]

Comment on attachment 115428 [details] [diff] [review]
first candidate

>Index: nsIStringEnumerator.idl

>+ * Portions created by the Initial Developer are Copyright (C) 2___

probably safe to fill in the blank now ;-)


>+interface nsIStringEnumeratorBase : nsISupports
>+{
>+    boolean hasMore();
>+};
>+interface nsIStringEnumerator : nsIStringEnumeratorBase
>+{
>+    AString getNext();
>+};
>+interface nsIUTF8StringEnumerator : nsIStringEnumeratorBase
>+{
>+    AUTF8String getNext();
>+};

i'd prefer to have just two interfaces because from a users point of
view there doesn't seem to be much value to the nsIStringEnumeratorBase
interface by itself.  supporting this interface means an additional
QI, etc.  i imagine you are doing this so you can have one class impl
both interfaces.  we have a similar situation with classes that wish
to support both nsIInputStream and nsIOutputStream (both have a Close
method).  you can see how this is handled in nsPipe3.cpp for example.
you might consider something similar so the interfaces can be cleaner.
or maybe it's not worth it :-/


>Index: nsStringEnumerator.h

>+#include "nsVoidArray.h"

can you do without this #include?  (maybe i missed where it is needed)


are you sure you don't want to call this NS_NewUTF8StringEnumerator instead?
sure C++ lets you call them the same thing, but it might help make code more
self documenting (and help folks keep on top of what charset they have) if
you add UTF8 to the name.

>+>+NS_COM nsresult
>+NS_NewStringEnumerator(nsIUTF8StringEnumerator** aResult,
>+                       const nsCStringArray* aArray,
>+                       PRBool aOwnsArray = PR_FALSE);
>+
>+NS_COM nsresult
>+NS_NewStringEnumerator(nsIStringEnumerator** aResult,
>+                       const nsStringArray* aArray,
>+                       PRBool aOwnsArray = PR_FALSE);

hmm.. what if nsStringArray supported shallow copy?  then
users could allocate nsStringArray on the stack, avoiding
a heap allocation for the array container.

it's also a bit icky to talk about calling delete on a 
pointer declared const.  (i know you're declaring it const
to make this interface work with both const and non-const,
but it's shame to have to abuse constructs like this.)


>+// Copying enumerators make a copy of all the strings in question,
>+// essentially a snapshot of the current state of the array. The
>+// implementation is memory efficient, making a single allocation to
>+// hold the enumerator and all of its strings.

this feels like something a string array should be able to do.	then
you would only need a dependent enumerator.

also, do you know of any instances in the tree where someone actually
wants to modify an array as they are enumerating it?  copying the array
is usually not the best solution to such a problem.  e.g., sometimes
you can enumerate backwards if all you might do is remove the current
element or append to the end of the list or just after the iterator.

i worry that folks will use a copying enumerator when they could of
just used a smarter form of a non-copying enumerator.


>Index: nsStringEnumerator.cpp

let's agree on the interfaces first :)

Comment 14

[Darin Fisher]

>hmm.. what if nsStringArray supported shallow copy?  then
>users could allocate nsStringArray on the stack, avoiding
>a heap allocation for the array container.

i think this is actually a non-issue.  this works great:

  {
     nsStringArray array;
     // build up array
     NS_NewStringEnumerator(getter_AddRefs(enumerator), &array, PR_FALSE);
  }

but i'm still bothered by saying that this thing is going to delete a |const|
pointer when aOwnsArray == TRUE.

Comment 15

[Alec Flett]

darin - the issue with making a full fledged copy of the array is that there's
nowguarantee of ownership since nsStringArrays aren't refcounted. (i.e. it isn't
so much about the array getting modified) 

so the internal nsStringArray could get deleted before the enumerator goes away
- but they still need to own the nsStringArray for their purposes. some of the
consumers are going to require this, such as the nsIParam object in bug 194289
(blocked on this bug)

But as for the names of the NS_* methods, I'm flexible. In fact I'm debating
adding the "Copying" to some of them to indicate exactly what they're doing...

And finally as for deleting "const" nsStringArrays - I know its ugly. I could
split it into two methods:

>+NS_COM nsresult
>+NS_NewStringEnumerator(nsIStringEnumerator** aResult,
>+                       nsStringArray* aArray,
>+                       PRBool aOwnsArray = PR_FALSE);
>+
>+NS_COM nsresult
>+NS_NewStringEnumerator(nsIStringEnumerator** aResult,
>+                       const nsStringArray* aArray)

..where the 2nd method internally sets mOwnsArray to PR_FALSE. This
syntactically hides the possibility of deleting a const nsStringArray.

Comment 16

[Alec Flett]

Attachment: string enumerator implementation

darin convinced me to ditch the copying string enumerator... we discussed that
the real issue is that nsStringArray is not refcounted. We talked about how if
there were special ownership models that had to be adhered to, that the
clients/providers of such an enumerator should provide a custom enumerator.
After poking at the code a little more, I realized it is really easy to just
add an optional "owner" parameter to the constructors which allow the
enumerator to hold a reference to an XPCOM object which IS refcounted, which
will be responsible for destroying the array.

Here's the updated patch. Anyone want to review?

Comment 17

[Alec Flett]

Comment on attachment 116475 [details] [diff] [review]
string enumerator implementation

ok, here's the updated enumerator. Can I get some reviews? Thanks.

this is more or less the same as the last patch without the copying enumerator
and with the ownership object.

Comment 18

[Darin Fisher]

Comment on attachment 116475 [details] [diff] [review]
string enumerator implementation

>Index: ds/nsIStringEnumerator.idl

>+ * Portions created by the Initial Developer are Copyright (C) 2___

2003 :-)


>Index: ds/nsStringEnumerator.h

>+// Non-copying enumerators hold a pointer to the array. Be careful
>+// because modifying the array may confuse the iterator, especially if
>+// you insert or remove elements in the middle of the array.

reference to "non-copying" implies existance of "copying" version :(


>+NS_NewStringEnumerator(nsIStringEnumerator** aResult,
>+                       nsStringArray* aArray,
>+                       PRBool aOwnsArray = PR_FALSE);

if i don't want the enumerator to "own" the array, then why wouldn't i
just call the |const| versions of these methods?  seems like the boolean
parameter is redundant.  of course, without it you have C++ name conflicts :-/

maybe NS_NewAdoptingStringEnumerator???


>+NS_COM nsresult
>+NS_NewStringEnumerator(nsIStringEnumerator** aResult,
>+                       nsStringArray* aArray, nsISupports* aOwner);

shouldn't the array be |const| here as well?


also, should we define a nsICStringEnumerator?	it might be useful when
dealing strictly with ASCII data.  i'm a bit iffy myself about whether
or not this should be added.


>Index: ds/nsStringEnumerator.cpp

all the mIsUnicode branches make me think you should be defining
separate classes for each interface.  you could allocate them all
as one object and bind their AddRef, Release, and QueryInterface
much as is done with nsPipe, nsPipeInputStream, nsPipeOutputStream.
but, i'm not going to hold you to it ;-)


>+template<class T>
>+static nsresult
>+StringEnumeratorTail(T** aResult)

decl inline?


>+NS_NewStringEnumerator(nsIStringEnumerator** aResult,
>+                       nsStringArray* aArray, nsISupports* aOwner)
>+{
>+    NS_ENSURE_ARG_POINTER(aResult);
>+    NS_ENSURE_ARG_POINTER(aArray);

are runtime checks really required here?  why not just use debug assertions?

Comment 19

[Alec Flett]

I know there are a lot of branches, but it really didn't seem worth the effort
to make all these seperate classes. I had them as seperate classes at one point,
and there was just a lot of redundant code. 
I'll clean up the other stuff and reattach a patch.

Comment 20

[Alec Flett]

oh, by "worth the effort" I meant "worth the code bloat" - as I previously had
them seperated, I had clearly made the effort before :) 

There is just one branch per each GetNext() (which is the method that will be
called the most often) and its pretty cheap (just checking a boolean member
variable) - the only other branches are the one in the destructor and in Count()
- but I feel like if we're using nsIStringEnumerators, we can't be THAT
concerned about performance on a branch-counting level. (i.e. if performance is
a concern somewhere that uses nsIStringEnumerator, then they should come up with
some specialized non-COM mechanism or use IDL arrays)

Comment 21

[Darin Fisher]

alec: good point.  nevermind then ;-)

Comment 22

[Alec Flett]

Attachment: string enumerator v1.2

ok, here's what I think is the final version. I didn't add an
nsICStringEnumerator because the nsIUTF8StringEnumerator will behave exactly
like a cstring-version as long as you give it an nsCStringArray - no conversion
will be performed. When crossing language boundaries also shouldn't cause
problems as long as we're dealing with 7-bit ASCII. 

For now I'd like to stick to what we have and see if there is really a need
down the road. (i.e. where crossing language boundaries causes bad conversions)

Comment 23

[Darin Fisher]

Comment on attachment 116778 [details] [diff] [review]
string enumerator v1.2

>+template<class T>
>+inline nsresult
>+StringEnumeratorTail(T** aResult)

i would have said "static inline" just for good measure... don't want some
compiler/linker thinking it should do extra work to possibly support an extern
decl / separate instance of this method in some other translation unit :-/

sr=darin

Comment 24

[Alec Flett]

Comment on attachment 116778 [details] [diff] [review]
string enumerator v1.2

thanks I'll make it static inline.. over to dougt.

Comment 26

[Alec Flett]

thanks guys... the fix is now in.