Last Comment Bug 239716 - inout wstring methods are difficult to implement with nsEmbedString
: inout wstring methods are difficult to implement with nsEmbedString
Status: RESOLVED FIXED
: fixed1.7
Product: Core
Classification: Components
Component: String (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Marco Pesenti Gritti
:
Mentors:
Depends on:
Blocks: 239789
  Show dependency treegraph
 
Reported: 2004-04-05 11:09 PDT by Marco Pesenti Gritti
Modified: 2007-05-18 01:24 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed fix (11.69 KB, patch)
2004-04-19 14:21 PDT, Marco Pesenti Gritti
darin.moz: review-
Details | Diff | Splinter Review
same patch using -w (8.30 KB, patch)
2004-04-19 14:22 PDT, Marco Pesenti Gritti
no flags Details | Diff | Splinter Review
add offset check (10.90 KB, patch)
2004-04-19 15:59 PDT, Marco Pesenti Gritti
no flags Details | Diff | Splinter Review
Let's try to get it in 1.7 -> fix the offsets (11.38 KB, patch)
2004-04-19 16:30 PDT, Marco Pesenti Gritti
darin.moz: review-
Details | Diff | Splinter Review
get rid of the 1.8 comment (11.38 KB, patch)
2004-04-19 16:39 PDT, Marco Pesenti Gritti
no flags Details | Diff | Splinter Review
make changes requested by darin (11.37 KB, patch)
2004-04-19 17:03 PDT, Marco Pesenti Gritti
darin.moz: review+
dbaron: superreview+
dbaron: approval1.7+
Details | Diff | Splinter Review

Description Marco Pesenti Gritti 2004-04-05 11:09:19 PDT
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) 2004-04-05 11:14:27 PDT
>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 2004-04-05 11:21:55 PDT
Yeah, typo. Sorry.
Comment 3 Marco Pesenti Gritti 2004-04-11 09:34:15 PDT
darin, does the proposed api make sense to you ? If so I can try to make a patch.
Comment 4 Darin Fisher 2004-04-11 13:33:08 PDT
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 2004-04-11 13:36:26 PDT
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 2004-04-11 13:47:52 PDT
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 2004-04-11 13:57:50 PDT
Hrm I missed your first comment. That would work for me ...
Comment 8 Marco Pesenti Gritti 2004-04-19 14:21:49 PDT
Created attachment 146522 [details] [diff] [review]
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 2004-04-19 14:22:47 PDT
Created attachment 146523 [details] [diff] [review]
same patch using -w
Comment 10 Darin Fisher 2004-04-19 14:46:52 PDT
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 2004-04-19 15:59:04 PDT
Created attachment 146535 [details] [diff] [review]
add offset check
Comment 12 Marco Pesenti Gritti 2004-04-19 16:30:37 PDT
Created attachment 146536 [details] [diff] [review]
Let's try to get it in 1.7 -> fix the offsets
Comment 13 Marco Pesenti Gritti 2004-04-19 16:39:06 PDT
Created attachment 146539 [details] [diff] [review]
get rid of the 1.8 comment
Comment 14 Darin Fisher 2004-04-19 16:39:28 PDT
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 2004-04-19 17:03:03 PDT
Created attachment 146540 [details] [diff] [review]
make changes requested by darin
Comment 16 Darin Fisher 2004-04-19 17:06:10 PDT
Comment on attachment 146540 [details] [diff] [review]
make changes requested by darin

r=darin
Comment 17 Marco Pesenti Gritti 2004-04-19 17:07:16 PDT
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+2 (review requests must explain patch) 2004-04-19 17:12:02 PDT
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 2004-04-19 17:29:06 PDT
fixed-on-trunk and fixed1.7

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