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.

> 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)?

(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.

> 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

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*...)

> 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.).

how about something like this?

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?

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 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?

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.

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 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

> 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.

revised patch

> I'd rather not add a runtime check for something so obscure. ok. but maybe an assertion would be a good idea, then.

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.

dbaron deferred to bsmedberg for SR on this patch.

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

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

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.