inout wstring methods are difficult to implement with nsEmbedString

RESOLVED FIXED

Status

()

Core
String
RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: Marco Pesenti Gritti, Assigned: Marco Pesenti Gritti)

Tracking

({fixed1.7})

Trunk
x86
Linux
fixed1.7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

14 years ago
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.
>NS_STRINGAPI(PRUnichar*)
>NS_StringCloneData (const nsCString &aStr);

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

right?
(Assignee)

Comment 2

14 years ago
Yeah, typo. Sorry.
(Assignee)

Updated

14 years ago
Blocks: 239789
(Assignee)

Comment 3

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

Comment 4

14 years ago
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

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

Comment 6

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

Comment 7

14 years ago
Hrm I missed your first comment. That would work for me ...
(Assignee)

Comment 8

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

Comment 9

14 years ago
Created attachment 146523 [details] [diff] [review]
same patch using -w
(Assignee)

Updated

14 years ago
Attachment #146522 - Flags: review?(darin)

Comment 10

14 years ago
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.
Attachment #146522 - Flags: review?(darin) → review-
(Assignee)

Comment 11

14 years ago
Created attachment 146535 [details] [diff] [review]
add offset check
Attachment #146522 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #146535 - Flags: review?(darin)
(Assignee)

Comment 12

14 years ago
Created attachment 146536 [details] [diff] [review]
Let's try to get it in 1.7 -> fix the offsets
Attachment #146535 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #146536 - Flags: review?(darin)
(Assignee)

Updated

14 years ago
Attachment #146535 - Flags: review?(darin)
(Assignee)

Comment 13

14 years ago
Created attachment 146539 [details] [diff] [review]
get rid of the 1.8 comment
Attachment #146536 - Attachment is obsolete: true

Comment 14

14 years ago
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!
Attachment #146536 - Attachment is obsolete: false
Attachment #146536 - Flags: review?(darin) → review-
(Assignee)

Comment 15

14 years ago
Created attachment 146540 [details] [diff] [review]
make changes requested by darin
(Assignee)

Updated

14 years ago
Attachment #146536 - Attachment is obsolete: true
Attachment #146539 - Attachment is obsolete: true

Comment 16

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

r=darin
Attachment #146540 - Flags: review+
(Assignee)

Comment 17

14 years ago
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
Attachment #146540 - Flags: superreview?(dbaron)
Attachment #146540 - Flags: approval1.7?
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.
Attachment #146540 - Flags: superreview?(dbaron)
Attachment #146540 - Flags: superreview+
Attachment #146540 - Flags: approval1.7?
Attachment #146540 - Flags: approval1.7+

Comment 19

14 years ago
fixed-on-trunk and fixed1.7
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Keywords: fixed1.7
Resolution: --- → FIXED

Updated

10 years ago
Assignee: string → mpgritti
You need to log in before you can comment on or make changes to this bug.