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

Added support for wchar_t and std::wstring for C# #983

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zillemarco
Copy link
Contributor

With the help of @tritao:

  • added support for wchar_t
  • added support for std::wstring
  • unified the way wide character strings are handled between C# and
    C++/CLI
  • changed the way strings are handled, using 'IntPtr' instead of 'string'
    with marshalling attributes on the P/Invokes

@@ -20,5 +20,15 @@ public static string MarshalEncodedString(IntPtr ptr, Encoding encoding)

return encoding.GetString(buffer);
}

public static IntPtr StringToHGlobalMultiByteUni(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.

This isn't used anywhere.

@@ -68,7 +68,11 @@ public override bool VisitFunctionDecl(Function function)

// Deleting destructors (default in v-table) accept an i32 bitfield as a
// second parameter in MS ABI.
if (method != null && method.IsDestructor && Context.ParserOptions.IsMicrosoftAbi)
var @class = method != null ? method.Namespace as Class : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Needlessly complicated - if the method is not null, then its name-space is always a class, just cast it.

var type = ctx.Parameter.Type.Desugar();
ClassTemplateSpecialization basicString = GetBasicString(type);

if(basicString.Arguments[0].Type.Type.IsPrimitiveType(PrimitiveType.Char))
Copy link
Contributor

Choose a reason for hiding this comment

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

A space after the conditional operator.

// Struct added to help map a the wchar_t C++ type to C#
// The data is stored as a 64 bit value so it could represent
// an UTF-32 character but C# only represents UTF-16 characters
// so beware of possible data loss when using it.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what this means, is it that our binding for wstring does not always work properly?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just that C# char (UTF-16) type cannot represent the entirety of Unicode (UTF-32).

Copy link
Contributor

Choose a reason for hiding this comment

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

We should then think about throwing an exception if out of range.

Copy link

Choose a reason for hiding this comment

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

I don't get this comment. wchar_t on Windows is represented as UTF16-LE. On Linux it's UTF-32. Char in C# is always UTF16-LE afaik. There is one more type to consider: char16_t is always UTF16-LE, regardless of the platform that's being used. (Since C++17). C++20 will also define UTF8 type char8_t.

@dnfadmin
Copy link

dnfadmin commented Sep 5, 2020

CLA assistant check
All CLA requirements met.

@tritao
Copy link
Collaborator

tritao commented Sep 5, 2020

It's not compiling anymore:

Passes\IgnoreSystemDeclarationsPass.cs(60,49): error CS0103: The name 'GetCharSpecializations' does not exist in the current context [C:\projects\cppsharp\src\Generator\CppSharp.Generator.csproj]

Guess some code moved around or was renamed, and needs to be updated in this PR.

@zillemarco
Copy link
Contributor Author

Yeah I know, during the rebase I had quite a bit of conflicts. I was thinking about drafting the PR until I get it back to work.

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.

6 participants