243484 - Add PRUint32 length out param to ToNewUTF8String() and UTF8ToNewUnicode()

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Johnny Stenback (:jst)]

Right now it's impossible to create a new UTF-8 string (char *) from a UTF-16
string and know the length of the resulting UTF-8 string w/o extra work. Same
goes for going the other way. This could be solved by adding a versions of
ToNewUTF8String() and UTF8ToNewUnicode() that take a PRUint32 out param that
gets the length of the result. Patch coming up.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Add PRUint32 out params.

This essentially adds new methods that take the out param, and makes the
methods with the old signature inline, calling the new methods with a null out
param.

Comment 2

[Darin Fisher]

Comment on attachment 148381 [details] [diff] [review]
Add PRUint32 out params.

nit: you say that these extra return values give character counts, but in
reality they specify the number of storage units returned.  i.e., the number of
bytes in the case of aUTF8Count, and the number of double-bytes in the case of
aUTF16Count.

otherwise, it looks good to me.

i presume that you are thinking of places where this extra argument would be
especially handy, right?

sr=darin with that comment change (and assuming dbaron is happy)

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 148381 [details] [diff] [review]
Add PRUint32 out params.

The comments definitely need to be fixed, as darin said.  But also, why not use
default params rather than extra inline methods?

Comment 4

[Johnny Stenback (:jst)]

I initially did that, but that'll change the signature of the methods, so
external libraries compiled with current XPCOM won't like XPCOM after this
change lands. If we don't care about that, I'm happy to change to default
parameters.

The comment now talks about "8-bit code units" and "16-bit code units", taken
from the CopyUnicodeTo() comments. Good enough?

Comment 5

[Darin Fisher]

(In reply to comment #4)
> I initially did that, but that'll change the signature of the methods, so
> external libraries compiled with current XPCOM won't like XPCOM after this
> change lands. If we don't care about that, I'm happy to change to default
> parameters.

changing the signature of the methods is probably a good thing...

consider what happens if these methods are called without anything passed for
the second argument.  then our new functions will possibly dereference a random
address and assign to that address (assuming the random value passed in is not 0).


> The comment now talks about "8-bit code units" and "16-bit code units", taken
> from the CopyUnicodeTo() comments. Good enough?

yeah, sounds good.  "storage units" is also used in other places.

Comment 6

[Darin Fisher]

also, these are not frozen functions :-)

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

How does this patch not change the signature of the methods?

Comment 8

[Johnny Stenback (:jst)]

With this patch char *ToNewUTF8String(const nsAString& aSource) still exists,
with it's old signature. Doesn't NS_COM force it to be available as a non-inline
method in XPCOM?

If not, then I'll just change the signature and call it a day.

Comment 9

[Darin Fisher]

ok, mixing inline with NS_COM is a bad idea :)

i think the cold medicine is getting to me.... thanks dbaron!

Comment 10

[Johnny Stenback (:jst)]

Ok, I'm on crack. New patch coming up.

Comment 11

[Johnny Stenback (:jst)]

Attachment: Use default arguments, no more inlines

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 148393 [details] [diff] [review]
Use default arguments, no more inlines

r=dbaron, although I probably prefer just "units" to "code units"

Comment 13

[Johnny Stenback (:jst)]

Fix checked in. I've got a patch to make XPConnect use this too, I'll roll that
in here as well.

Comment 14

[Johnny Stenback (:jst)]

Attachment: Make XPConnect use UTF8ToNewUnicode() now that it can w/o doing extra work to get the string length.

Comment 15

[Peter Van der Beken [:peterv]]

I think you could fix this one too:
http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsVariant.cpp#1110

Comment 16

[Darin Fisher]

Comment on attachment 148440 [details] [diff] [review]
Make XPConnect use UTF8ToNewUnicode() now that it can w/o doing extra work to get the string length.

>Index: js/src/xpconnect/src/xpcconvert.cpp

>+                    jschar *p = (jschar *)UTF8ToNewUnicode(*cString, &len);
...
>+                    JSString* jsString = JS_NewUCString(cx, p, len);

hmm... doesn't this technically mix allocators?  should you use
JS_NewExternalString
with a finalizer that calls nsMemory::Free?  or do we already make assumptions
in
XPConnect that nsMemory::Free is equivalent to |free|.	brendan says "yes, we
do",
so i guess we can ignore this issue here.

r+sr=darin

Comment 17

[Johnny Stenback (:jst)]

Attachment: nsVariant.cpp string code speedup, and other cleanup.

Comment 18

[Johnny Stenback (:jst)]

nsVariant.cpp changes checked in now too. Marking FIXED.

Comment 19

[Brendan Eich [:brendan]]

Mix allocators... *snort*.

We should get rid of bogus layering on malloc and free.

/be

Comment 20

[Darin Fisher]

yeah, nsMemory::* should only be used by external code :-)