Last Comment Bug 264274 - support dependent strings in frozen string API
: support dependent strings in frozen string API
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: String (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla1.8beta1
Assigned To: Darin Fisher
:
:
Mentors:
Depends on:
Blocks: 205425 266974 268520 270110
  Show dependency treegraph
 
Reported: 2004-10-13 16:56 PDT by Darin Fisher
Modified: 2004-11-16 08:12 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
prototype patch (3.05 KB, patch)
2004-10-26 19:56 PDT, Darin Fisher
no flags Details | Diff | Splinter Review
v1 patch (22.89 KB, patch)
2004-10-30 21:44 PDT, Darin Fisher
no flags Details | Diff | Splinter Review
v1.1 patch (23.84 KB, patch)
2004-11-05 23:37 PST, Darin Fisher
cbiesinger: review+
Details | Diff | Splinter Review
v1.2 patch (24.44 KB, patch)
2004-11-06 13:08 PST, Darin Fisher
benjamin: superreview+
Details | Diff | Splinter Review

Description Darin Fisher 2004-10-13 16:56:08 PDT
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) 2004-10-13 17:14:37 PDT
> 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 2004-10-13 17:58:56 PDT
(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] 2004-10-13 18:33:06 PDT
> 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) 2004-10-15 05:16:28 PDT
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 2004-10-15 10:10:56 PDT
> 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 2004-10-26 19:56:36 PDT
Created attachment 163505 [details] [diff] [review]
prototype patch

how about something like this?
Comment 7 Darin Fisher 2004-10-26 19:59:25 PDT
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 2004-10-30 21:44:54 PDT
Created attachment 164059 [details] [diff] [review]
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) 2004-11-01 17:38:48 PST
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 2004-11-01 19:08:50 PST
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 2004-11-05 23:37:55 PST
Created attachment 164818 [details] [diff] [review]
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) 2004-11-06 07:21:40 PST
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 2004-11-06 12:58:38 PST
> 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 2004-11-06 13:08:52 PST
Created attachment 164916 [details] [diff] [review]
v1.2 patch

revised patch
Comment 15 Christian :Biesinger (don't email me, ping me on IRC) 2004-11-06 15:35:14 PST
> 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 2004-11-07 22:42:10 PST
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 2004-11-15 14:15:53 PST
dbaron deferred to bsmedberg for SR on this patch.
Comment 18 Benjamin Smedberg [:bsmedberg] 2004-11-15 19:30:58 PST
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 2004-11-15 20:45:56 PST
fixed-on-trunk

will create nsString and nsDependentString (and possibly other utility classes)
as discussed in another bug.
Comment 20 Marco Pesenti Gritti 2004-11-16 02:24:32 PST
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.

Note You need to log in before you can comment on or make changes to this bug.