Closed Bug 264274 Opened 20 years ago Closed 20 years ago

support dependent strings in frozen string API

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.8beta1

People

(Reporter: darin.moz, Assigned: darin.moz)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8beta
> 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.).
Attached patch prototype patch (obsolete) — Splinter Review
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?
Attached patch v1 patch (obsolete) — Splinter Review
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.
Attachment #163505 - Attachment is obsolete: true
Attachment #164059 - Flags: review?(dbaron)
Blocks: 266974
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.
Attached patch v1.1 patch (obsolete) — Splinter Review
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.
Attachment #164059 - Attachment is obsolete: true
Attachment #164059 - Flags: review?(dbaron)
Attachment #164818 - Flags: review?(cbiesinger)
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
Attachment #164818 - Flags: review?(cbiesinger) → review+
> 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.
Attached patch v1.2 patchSplinter Review
revised patch
Attachment #164818 - Attachment is obsolete: true
Attachment #164916 - Flags: superreview?(dbaron)
> 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.
Blocks: 268520
Blocks: 205425
Attachment #164916 - Flags: superreview?(dbaron) → superreview?(bsmedberg)
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
Attachment #164916 - Flags: superreview?(bsmedberg) → superreview+
fixed-on-trunk

will create nsString and nsDependentString (and possibly other utility classes)
as discussed in another bug.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
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.
Blocks: 270110
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: