264274 - support dependent strings in frozen string API

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Darin Fisher]

Support dependent strings in frozen string API.

I think it was a mistake not to have added an API for this back when we
originally created nsStringAPI.h for mozilla 1.7.  I'd like to propose the
following pair of methods:

  NS_STRINGAPI(nsresult)
  NS_StringContainerInitDep(nsStringContainer &container,
                            const PRUnichar *data,
                            PRUint32 dataLen = PR_UINT32_MAX);

  NS_STRINGAPI(nsresult)
  NS_CStringContainerInitDep(nsCStringContainer &container,
                             const char *data,
                             PRUint32 dataLen = PR_UINT32_MAX);

When a string container is initialized using one of these methods, we'll say
that it is not necessary to call NS_C?StringContainerFinish.  If dataLen is
PR_UINT32_MAX, then the string length is computed automatically.  Also, data
must be null terminated.  (We may want to add a flag to indicate whether or
not data is null terminated, in case we also wanted to support dependent 
substrings.)

The two main issues this will improve:

  o  enables support for a more efficient version of the NS_LITERAL_C?STRING 
     macro.  Today, one must write:

       #define NS_LITERAL_STRING(x) nsEmbedString(L##x)

     This example of course is limited to platforms where the 'L' prefix makes
     sense, but that includes Linux and Windows, so it pretty much means all of
     our embedding customers.  Today's solution means a heap allocation :-(

  o  enables support for a more efficient charset conversion when starting
     with a raw character array pointer.  Today, one must write:
   
       nsEmbedString result; 
       NS_CStringToUTF16(nsEmbedCString(charPtr), NS_CSTRING_ENCODING_UTF8,
                         result);

     which results in an intermediate heap allocated copy of the contents of 
     charPtr.

Comment 1

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

> we'll say that it is not necessary to call NS_C?StringContainerFinish. 

but it's allowed?

will you also some kind of support for this to nsEmbedC?String (to support the
use cases you cite)?

Comment 2

[Darin Fisher]

(In reply to comment #1)
> > we'll say that it is not necessary to call NS_C?StringContainerFinish. 
> 
> but it's allowed?

sure, it would be permitted.  basically, the destructor for nsDependentString
does nothing, provided that nsDependentString is not mutated.  hmm... that could
be an issue, and it might be reason enough to require the call to *Finish :-(


> will you also some kind of support for this to nsEmbedC?String (to support the
> use cases you cite)?

yes, indeed.  nsDependentEmbedString? -- icky name, but something like that i guess.

i'm also tempted to just use the names: "nsString" and "nsDependentString",
making nsString be what nsEmbedString is today and relegating nsEmbedString to a
deprecated typedef.

Comment 3

[Brendan Eich [:brendan]]

> i'm also tempted to just use the names: "nsString" and "nsDependentString",
> making nsString be what nsEmbedString is today and relegating nsEmbedString to
> a deprecated typedef.

Do it! ;-)

/be

Comment 4

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

hmm, it might cause confusion if there are two entirely different classes named
nsString...

(hmm, the two uses cases you cite could also be implemented by having
NS_CStringToUTF16 take a const char*...)

Comment 5

[Darin Fisher]

> hmm, it might cause confusion if there are two entirely different classes named
> nsString...

I'm not so worried about that.  We already define nsAString and nsACString
differently in the Gecko SDK.  People coding against the SDK should only have to
think about what's in the SDK.  If that overlaps with idioms inside Mozilla,
that's ok... and sometimes good if the idioms match up.  So, I don't see it as a
problem.


> (hmm, the two uses cases you cite could also be implemented by having
> NS_CStringToUTF16 take a const char*...)

yeah, but that doesn't scale well... there's a reason why we have
nsDependentString in the tree today.  we don't want to create alternate versions
of all our APIs (|const nsACString&| vs. |const char*|, etc.).

Comment 6

[Darin Fisher]

Attachment: prototype patch

how about something like this?

Comment 7

[Darin Fisher]

it might make sense to extend the concept of NS_StringContainerInit2 with a
NS_StringSetData2 that also permits assigning a shared buffer, an adopted
buffer, and permits specifying null-terminated vs. string fragment.  in which
case the flags should be named differently.

thoughts?

Comment 8

[Darin Fisher]

Attachment: v1 patch

This patch adds the new API.  It includes test cases in TestMinStringAPI.cpp,
but it does not yet include any changes to the helper classes (nsEmbedString,
etc.) that are included in the SDK.  That will be done as a separate patch.

Comment 9

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

Comment on attachment 164059 [details] [diff] [review]
v1 patch

- It seems to me that you should need the flag only if you want the less common
behaviour, i.e. if you want an unterminated string

- " this parameter is ignored when aData
+ *			 is null."
+ * NOTE: NS_CStringContainerInit2(container, nsnull, 0, 0) is equivalent to
+ * NS_CStringContainerInit(container).

this doesn't seem to be implemented - you do in fact assert and return an error
if you get 0 as flags.

in totally different news, is it mentioned anywhere that string containers may
be initialized only once?

Comment 10

[Darin Fisher]

biesi: thanks for the feedback.  i agree that terminated should be the default
mode, and i'll fix the code so that it indeed conforms to my documentation ;-)


> in totally different news, is it mentioned anywhere that string containers may
> be initialized only once?

uhm... well, it does say that you must call NS_StringContainerFinish to release
any memory owned by the nsStringContainer.  but, i don't think it says anything
about what happens if you call Init twice.  we should flag that as an invalid
operation.  i don't think we can reasonably test for that condition, so it
should only be stated that calling Init twice without calling Finish in between
is a no-no, resulting in uncertain badness.

Comment 11

[Darin Fisher]

Attachment: v1.1 patch

revised patch.	i know that bsmedberg's xpcom allocator patch is going to
conflict with this one, but i wanted to post a revised version of this one for
review asap.

Comment 12

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

Comment on attachment 164818 [details] [diff] [review]
v1.1 patch

tests/TestMinStringAPI.cpp
+    const char kData[] = "hello world";

make it a static const char? not like it matters.

being a test program, should it check the return value of these NS_CString*
methods?

I wonder if this should test that invalid flag combinations return an error...


hm, should passing DEPEND|ADOPT return an error? (because this combination
doesn't seem to make sense)

I wonder what happens when someone passes a non-null buffer with length=0 :)

r=biesi if you get sr=dbaron

Comment 13

[Darin Fisher]

> make it a static const char? not like it matters.

yeah, it doesn't really matter ;)


> being a test program, should it check the return value of these NS_CString*
> methods?

yeah, that would be a good idea.


> I wonder if this should test that invalid flag combinations return an error...

hmm... perhaps.


> hm, should passing DEPEND|ADOPT return an error? (because this combination
> doesn't seem to make sense)

I'll add a comment that says that the two flags cannot be combined.  I'd 
rather not add a runtime check for something so obscure.  The implementation
has it so that ADOPT overrides DEPEND.


> I wonder what happens when someone passes a non-null buffer with length=0 :)

Same thing as |s.Assign("foo", 0)|, which is equivalent to |s.Truncate()|


Thanks for the quick review.

Comment 14

[Darin Fisher]

Attachment: v1.2 patch

revised patch

Comment 15

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

> I'd rather not add a runtime check for something so obscure.

ok. but maybe an assertion would be a good idea, then.

Comment 16

[Darin Fisher]

biesi: ok, i'll add the assertion, but most extension authors or embedders will
probably never see assertions since they are compiled out of release builds.

Comment 17

[Darin Fisher]

dbaron deferred to bsmedberg for SR on this patch.

Comment 18

[Benjamin Smedberg]

Comment on attachment 164916 [details] [diff] [review]
v1.2 patch

r=me. Of course you need to conflict-fix this, but that's just moving stuff
around

Comment 19

[Darin Fisher]

fixed-on-trunk

will create nsString and nsDependentString (and possibly other utility classes)
as discussed in another bug.

Comment 20

[Marco Pesenti Gritti]

darin: if you could cc me on the other bug I'd appreciate. It should also be
marked as a blocker for bug 205425 instead of this one.