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

Fix behavior of SilkMarshal.StringToPtr and related methods on Linux #2377

Merged
merged 11 commits into from
Dec 7, 2024

Conversation

Exanite
Copy link
Contributor

@Exanite Exanite commented Dec 6, 2024

Summary of the PR

This fixes #2374 by checking for whether the platform is Windows and using 2-byte LPWStrs if it is Windows and 4-byte LPWStrs if it is not.

Related issues, Discord discussions, or proposals

#2374

Further Comments

This pull request includes the changes made in #2376.
In that pull request, I added a test project (Silk.NET.Core.Tests) which is used here to test the changes I made.

@Exanite Exanite force-pushed the fix/lpwstr-behavior-on-linux branch 2 times, most recently from a597187 to f5de3bc Compare December 6, 2024 16:12
@Exanite Exanite force-pushed the fix/lpwstr-behavior-on-linux branch from f5de3bc to af1f79d Compare December 6, 2024 16:18
@Exanite Exanite marked this pull request as ready for review December 6, 2024 16:21
@Exanite Exanite requested a review from a team as a code owner December 6, 2024 16:21
@Exanite Exanite marked this pull request as draft December 6, 2024 16:24
@Exanite
Copy link
Contributor Author

Exanite commented Dec 6, 2024

I built the package locally and overrode the package version that I was using.
image

Doing this makes my project run successfully on both Windows and Linux without any further changes.
I also ran the test cases I wrote on both Windows and Linux. The CI Build seems to take care of Mac.

@Exanite Exanite marked this pull request as ready for review December 6, 2024 17:22
Copy link
Member

@Perksey Perksey left a comment

Choose a reason for hiding this comment

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

Just a few comments, looks great!

src/Core/Silk.NET.Core/Native/SilkMarshal.cs Outdated Show resolved Hide resolved
src/Core/Silk.NET.Core/Native/SilkMarshal.cs Show resolved Hide resolved
@Exanite
Copy link
Contributor Author

Exanite commented Dec 6, 2024

I changed the code to use Encoding.UTF32 for both converting to a pointer and from a pointer. The test cases pass, but I'm still wondering if this is correct (see my reply to your review comment).

I also don't test for any edge cases. I'm considering about including some of those.

@Perksey Perksey enabled auto-merge (squash) December 6, 2024 19:21
@Perksey Perksey disabled auto-merge December 6, 2024 19:21
@Perksey
Copy link
Member

Perksey commented Dec 6, 2024

Will merge following #2376

@Exanite
Copy link
Contributor Author

Exanite commented Dec 6, 2024

Hold on. I want to do a bit more research and make sure the behavior here is correct.

@Exanite Exanite marked this pull request as draft December 6, 2024 20:08
@Exanite
Copy link
Contributor Author

Exanite commented Dec 6, 2024

Interesting. It looks like on Windows, LPWStr is UTF-16 as expected. However, on Linux, LPWStr is UTF-32.

For example, the memory for 🧵:

  • On Linux: 0xf5f901
  • On Windows: 0x3ed8f5dd

I checked this by inspecting the memory of L"🧵" on both Windows and Linux.

The codepoint for 🧵 is U+1F9F5.
The surrogate pair for 🧵 is 0xd8e3 and 0xddf5.

Ignoring byte order, everything matches up.

This means that it is indeed correct to use UTF32 on non-Windows platforms.
I'll update the code to reflect this.

@Exanite Exanite marked this pull request as ready for review December 6, 2024 21:57
@Exanite Exanite marked this pull request as draft December 6, 2024 21:59
@Exanite
Copy link
Contributor Author

Exanite commented Dec 6, 2024

I found another issue where on Windows, characters like emojis get garbled.
This apparently was always the case.

I'll look into fixing this as well.

image

This is because LPStr doesn't always support non-ascii characters.
@Exanite
Copy link
Contributor Author

Exanite commented Dec 7, 2024

Okay, never mind. That technically is expected behavior since that case is testing LPStr (which is 8 bits, but not necessarily UTF-8) and non-ascii characters are not guaranteed to be supported.

@Exanite Exanite marked this pull request as ready for review December 7, 2024 00:53
@Perksey Perksey merged commit ee1d2f0 into dotnet:main Dec 7, 2024
5 checks passed
@Exanite Exanite deleted the fix/lpwstr-behavior-on-linux branch December 8, 2024 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Behavior of SilkMarshal.StringToPtr and related methods on Linux
3 participants