239716 - inout wstring methods are difficult to implement with nsEmbedString

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Marco Pesenti Gritti]

There is no easy way to create a new unichar string.
The only way I found so far is:

  const PRUnichar* data;
  PRUint32 dataLen = NS_StringGetData(string, &data);
  return (PRUnichar *)nsMemory::Clone(string.get(), dataLen);

Maybe it would make sense to add

NS_STRINGAPI(PRUnichar*)
NS_StringCloneData (const nsAString &aStr);

NS_STRINGAPI(PRUnichar*)
NS_StringCloneData (const nsCString &aStr);

or inline version of these.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

>NS_STRINGAPI(PRUnichar*)
>NS_StringCloneData (const nsCString &aStr);

that should be:
NS_STRINGAPI(char*)
NS_StringCloneData (const nsACString &aStr);

right?

Comment 2

[Marco Pesenti Gritti]

Yeah, typo. Sorry.

Comment 3

[Marco Pesenti Gritti]

darin, does the proposed api make sense to you ? If so I can try to make a patch.

Comment 4

[Darin Fisher]

So this code is not technically valid:

  const PRUnichar* data;
  PRUint32 dataLen = NS_StringGetData(string, &data);
  return (PRUnichar *)nsMemory::Clone(string.get(), dataLen);

The result in this case is not a null-terminated string.  That means it cannot
be used as a |wstring| in/out-param.

The implementation needs to be something more like this:

PRUnichar *
NS_StringCloneData(const nsAString &aStr)
{
  const PRUnichar *data;
  PRUint32 len = NS_StringGetData(aStr, &data);
  
  PRUnichar *result = (PRUnichar *) nsMemory::Alloc((len+1)*sizeof(PRUnichar));
  if (!result)
    return nsnull;
  
  memcpy(result, data, len*sizeof(PRUnichar));
  result[len] = PRUnichar(0);
  return result;
}

We could provide this as a static inline method in nsStringAPI.h, or we could
add a new exported symbol from libxpcom.so for this function.  I'm not quite
sure which I prefer.

Comment 5

[Darin Fisher]

NOTE: if you have a nsEmbedString that you want to clone, then you could do this:

PRUnichar *
CloneEmbedString(const nsEmbedString &aStr)
{
  const PRUnichar *data;
  PRUint32 len = NS_StringGetData(aStr, &data);
  return (PRUnichar *) nsMemory::Clone(data, (len+1)*sizeof(PRUnichar));
}  

This works because the internal buffer of a nsEmbedString is always null-terminated.

Comment 6

[Marco Pesenti Gritti]

Yeah, I'm doing something like that right now. Though it's not very convenient,
that's why I posted the bug. Since this is a quite common usage I think it would
make sense to provide a "shortcut", either inline or not. Though it's up to you,
if you think it's unnecessary feel free to close the bug.

Comment 7

[Marco Pesenti Gritti]

Hrm I missed your first comment. That would work for me ...

Comment 8

[Marco Pesenti Gritti]

Attachment: proposed fix

I added symbols to xpcom. For the implementation I just used
ToNewUnicode/CString since that appear to already do what we want.
Going to attach a -w patch too because I had to realign some declarations.

Comment 9

[Marco Pesenti Gritti]

Attachment: same patch using -w

Comment 10

[Darin Fisher]

Comment on attachment 146522 [details] [diff] [review]
proposed fix

the code in nsXPComInit.cpp needs an offsetof check before assigning to
stringCloneData and cstringCloneData.

otherwise, this seems good to me.

Comment 11

[Marco Pesenti Gritti]

Attachment: add offset check

Comment 12

[Marco Pesenti Gritti]

Attachment: Let's try to get it in 1.7 -> fix the offsets

Comment 13

[Marco Pesenti Gritti]

Attachment: get rid of the 1.8 comment

Comment 14

[Darin Fisher]

Comment on attachment 146536 [details] [diff] [review]
Let's try to get it in 1.7 -> fix the offsets

>Index: build/nsXPCOMPrivate.h

>+    // Added for Mozilla 1.8
>+    StringCloneDataFunc stringCloneData;
>+    CStringCloneDataFunc cstringCloneData;

kill this comment.


>Index: glue/standalone/nsXPCOMGlue.cpp

> 
>-

this extra newline was intentional, used as a visual separator
between the NS_String and NS_CString functions.  please don't
remove it =)


>Index: string/public/nsStringAPI.h

> /**
>+ * NS_StringGetData
>+ *
>+ * This function returns a null-terminated copy of the string's
>+ * internal buffer.
>+ *
>+ * @param aStr          abstract string reference
>+ * @return              null-terminated copy of the string internal buffer
>+ * 
>+ * @status FROZEN
>+ */

please mention that the return value must be free'd using nsMemory::Free

replace "string" with "string's" in @return statement


r=darin with those changes.  thanks for the patch!

Comment 15

[Marco Pesenti Gritti]

Attachment: make changes requested by darin

Comment 16

[Darin Fisher]

Comment on attachment 146540 [details] [diff] [review]
make changes requested by darin

r=darin

Comment 17

[Marco Pesenti Gritti]

Comment on attachment 146540 [details] [diff] [review]
make changes requested by darin

dbaron, if you are ok with this it would be good to have sr and a together. The
offset check is correct only if this goes in 1.7

Comment 18

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

Comment on attachment 146540 [details] [diff] [review]
make changes requested by darin

sr=dbaron and a=dbaron.  I'm assuming the big reindentation shows only the two
one-line additions in a diff -w, like the diff -w above.

Comment 19

[Darin Fisher]

fixed-on-trunk and fixed1.7