The default bug view has changed. See this FAQ.

support dependent strings in frozen string API

RESOLVED FIXED in mozilla1.8beta1

Status

()

Core
String
--
enhancement
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: Darin Fisher, Assigned: Darin Fisher)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.8beta1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

13 years ago
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.
(Assignee)

Updated

13 years ago
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)?
(Assignee)

Comment 2

13 years ago
(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*...)
(Assignee)

Comment 5

13 years ago
> 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.).
(Assignee)

Comment 6

13 years ago
Created attachment 163505 [details] [diff] [review]
prototype patch

how about something like this?
(Assignee)

Comment 7

13 years ago
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?
(Assignee)

Comment 8

13 years ago
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.
Attachment #163505 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #164059 - Flags: review?(dbaron)
(Assignee)

Updated

13 years ago
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?
(Assignee)

Comment 10

13 years ago
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.
(Assignee)

Comment 11

13 years ago
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.
Attachment #164059 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #164059 - Flags: review?(dbaron)
(Assignee)

Updated

13 years ago
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+
(Assignee)

Comment 13

13 years ago
> 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.
(Assignee)

Comment 14

13 years ago
Created attachment 164916 [details] [diff] [review]
v1.2 patch

revised patch
Attachment #164818 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
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.
(Assignee)

Comment 16

13 years ago
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.
(Assignee)

Updated

13 years ago
Blocks: 268520

Updated

13 years ago
Blocks: 205425
(Assignee)

Updated

13 years ago
Attachment #164916 - Flags: superreview?(dbaron) → superreview?(bsmedberg)
(Assignee)

Comment 17

13 years ago
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+
(Assignee)

Comment 19

13 years ago
fixed-on-trunk

will create nsString and nsDependentString (and possibly other utility classes)
as discussed in another bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 20

13 years ago
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.
(Assignee)

Updated

13 years ago
Blocks: 270110
You need to log in before you can comment on or make changes to this bug.