Skip to content
This repository was archived by the owner on Nov 1, 2020. It is now read-only.

Add Out and Ref support for AnsiString and UnicodeString#3208

Merged
shrah merged 1 commit intodotnet:masterfrom
shrah:master
Apr 5, 2017
Merged

Add Out and Ref support for AnsiString and UnicodeString#3208
shrah merged 1 commit intodotnet:masterfrom
shrah:master

Conversation

@shrah
Copy link
Contributor

@shrah shrah commented Apr 4, 2017

No description provided.

@shrah
Copy link
Contributor Author

shrah commented Apr 4, 2017

@yzha @tijoytom @jkotas PTAL. This should fix #3203

public static unsafe string UnicodeBufferToString(char* buffer)
{

if (buffer == null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just return new String(buffer);?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

@yizhang82 yizhang82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo comments. Thanks!

{
get
{
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be only required if we don't pin.

// Marshalling a null string?
codeStream.Emit(ILOpcode.brfalse, lNullString);
// Marshalling a null string?
codeStream.Emit(ILOpcode.brfalse, lNullString);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check this earlier?

StoreManagedValue(codeStream);
}

protected override void EmitCleanupManagedToNative(ILCodeStream codeStream)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to change this to EmitCleanupManaged/EmitCleanupNative. It's easier to understand that way.


protected override void EmitCleanupManagedToNative(ILCodeStream codeStream)
{
if (!ShouldBePinned)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should make sure ShouldCleanup returns the right value - and you wouldn't have to special case here

{
get
{
return MarshallerType != MarshallerType.Field
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing a few cases. See our ilmarshallers.cpp:

bool ILWSTRMarshaler::CanUsePinnedManagedString(DWORD dwMarshalFlags)
{
LIMITED_METHOD_CONTRACT;
return IsCLRToNative(dwMarshalFlags) && !IsByref(dwMarshalFlags) && IsIn(dwMarshalFlags) && !IsOut(dwMarshalFlags);
}


int stringLength = str.Length;

if (stringLength == 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct. Empty string is not a null string. It should be a buffer '\0'.

}

char* buffer = (char*)PInvokeMarshal.CoTaskMemAlloc((UIntPtr)(sizeof(char) * (stringLength+1))).ToPointer();
for (int i = 0; i < stringLength; i++)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better copy operation? Buffer.copy?

unsigned short *p = expected;
unsigned short *q = *val;

while (*p && *q && *p == *q)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extract this into a helper?

return;

char *p = dst, *q = src;
while (*q)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strcpy?

private static extern int VerifyAnsiString(string str);

[DllImport("*", CallingConvention = CallingConvention.StdCall, CharSet = CharSet.Ansi)]
private static extern int VerifyAnsiStringOut(out string str);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another interesting case you might want to consider: [in] ref

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a case with [in] ref

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants