Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#64: String projection #71

Closed
wants to merge 1 commit into from
Closed

Conversation

saul
Copy link

@saul saul commented Jan 27, 2021

This fixes #64.

  • [In] strings are now projected to [MarshalAs(UnmanagedType.LPxStr)] string param
  • [Out] strings are now projected to [MarshalAs(UnmanagedType.LPxStr)] StringBuilder param

For example, CredUIPromptForCredentials is now projected to:

internal static extern unsafe uint CredUIPromptForCredentials(
    [In, Optional] CREDUI_INFOW*pUiInfo,
    [MarshalAs(UnmanagedType.LPWStr)] string pszTargetName,
    SecHandle*pContext,
    uint dwAuthError,
    [MarshalAs(UnmanagedType.LPWStr)] System.Text.StringBuilder pszUserName,
    uint ulUserNameBufferSize,
    [MarshalAs(UnmanagedType.LPWStr)] System.Text.StringBuilder pszPassword,
    uint ulPasswordBufferSize,
    [In, Out, Optional] int *save,
    uint dwFlags);

If the SizeParamIndex annotation is set on the parameter, then the overloaded method passes StringBuilder.Capacity as the buffer size.

I also did some cleanup to make it a bit easier to comprehend. I abstracted the two places where the NativeTypeInfoAttribute is read into a new method and represented it with a new NativeTypeInfo struct.

@jnm2
Copy link
Contributor

jnm2 commented Jan 31, 2021

Could you make it char* instead of StringBuilder? I never want to use StringBuilder in p/invoke signatures, and https://docs.microsoft.com/en-us/dotnet/standard/native-interop/best-practices#string-parameters also recommends against it.

To clarify: I'm hoping for the same overload to be able to have parameters string somethingIn, char* somethingOut.

Or string somethingIn, Span<char> somethingOut.

@saul
Copy link
Author

saul commented Jan 31, 2021

That's a really informative page - thanks for linking.

Out of interest why char* instead of char[], as the page suggests? I imagine the latter is a bit more ergonomic.

@jnm2
Copy link
Contributor

jnm2 commented Jan 31, 2021

I agree, string somethingIn, char[] somethingOut seems more ergonomic than char*.

If these types are available, I think this is more ergonomic still, because you can pass more than just char[] to the second parameter and more than just string (including char[]!) to the first parameter:
ReadOnlySpan<char> somethingIn, Span<char> somethingOut

@AArnott
Copy link
Member

AArnott commented Feb 2, 2021

The generated extern method should indeed be char*. This should then automatically lead to generation of an overload that takes a Span<char> or ReadOnlySpan<char> depending on the data direction.

char[] is implicitly castable to Span<char>, so using a span allows for many other scenarios, allowing reduced GC pressure and less memory copying.

@AArnott
Copy link
Member

AArnott commented Feb 2, 2021

BTW: the code cleanup that you mixed into this may cause the PR to take longer to merge. Ideally functional changes and cleanups would be in separate PRs for easier code reviews.
But I do appreciate you contributing!

@AArnott
Copy link
Member

AArnott commented Feb 17, 2021

Some other significant string changes are coming in a metadata update, so I'm going to take those and fix the bug in a more focused PR. @saul, I am interested in refactoring if it improves maintainability, but we should discuss in a github issue before you go to the trouble.

Thank you for your submission.

@AArnott AArnott closed this Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[In, Out] char* parameter MUST NOT generate string as a friendly overload
3 participants