Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Remove remaining StringBuilder marshaling use from Corelib #21120

Merged
stephentoub merged 2 commits into from
Nov 20, 2018
Merged

Remove remaining StringBuilder marshaling use from Corelib #21120

stephentoub merged 2 commits into from
Nov 20, 2018

Conversation

@stephentoub
Copy link
Member

@stephentoub stephentoub commented Nov 20, 2018

  • Unix globalization functions were using StringBuilder. Replaced them with stackallocs, as the max sizes were all relatively small.
  • Registry methods were declared to use StringBuilder, but these weren't actually used by corelib, and in fact, corefx isn't using StringBuilder with them either. I've changed the definitions for now to use char[] instead (all the call sites are passing in null), and I'll fix up some corefx usage separately.
  • Resource-related functions were using StringBuilder, and have been replaced with stackallocs, given reasonably small max buffer sizes.
  • ExpandEnvironmentVariables was using a StringBuilder, but it's rewritten equivalent code in corefx is not. For now I've copied the corefx function over to coreclr to replace the different implementation, but we can look at subsequently consolidating them into one.

cc: @jkotas, @JeremyKuhne, @eerhardt, @tarekgh

@stephentoub
Remove remaining StringBuilder marshaling use from Corelib
bcc973e 
- Unix globalization functions were using StringBuilder.  Replaced them with stackallocs, as the max sizes were all relatively small.
- Registry methods were declared to use StringBuilder, but these weren't actually used by corelib, and in fact, corefx isn't using StringBuilder with them either.  I've changed the definitions for now to use char[] instead (all the call sites are passing in null), and I'll fix up some corefx usage separately.
- Resource-related functions were using StringBuilder, and have been replaced with stackallocs, given reasonably small max buffer sizes.
- ExpandEnvironmentVariables was using a StringBuilder, but it's rewritten equivalent code in corefx is not.  For now I've copied the corefx function over to coreclr to replace the different implementation, but we can look at subsequently consolidating them into one.
jkotas
jkotas approved these changes Nov 20, 2018
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks for doing this

eerhardt
eerhardt reviewed Nov 20, 2018
View reviewed changes
src/System.Private.CoreLib/shared/Interop/Unix/System.Globalization.Native/Interop.Locale.cs
[DllImport(Libraries.GlobalizationNative, CharSet = CharSet.Unicode, EntryPoint = "GlobalizationNative_GetLocaleName")]
[return: MarshalAs(UnmanagedType.Bool)]
internal static extern unsafe bool GetLocaleName(string localeName, [Out] StringBuilder value, int valueLength);
internal static extern unsafe bool GetLocaleName(string localeName, char* value, int valueLength);
Copy link
Member

@eerhardt eerhardt Nov 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

(nit) I kind of like the documentation value that the [Out] attributes provides. But I'm totally fine with removing them here, since they can't be used with a pointer.

eerhardt
eerhardt approved these changes Nov 20, 2018
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

:shipit:

@stephentoub
Copy link
Member Author

stephentoub commented Nov 20, 2018

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test please (vector-related failures)

@stephentoub stephentoub merged commit a9b57bd into dotnet:master Nov 20, 2018
@stephentoub stephentoub deleted the sbmarshaling branch November 20, 2018 22:03
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@stephentoub
Remove remaining StringBuilder marshaling use from Corelib (dotnet/co…
a7f5ff6 
…reclr#21120)

* Remove remaining StringBuilder marshaling use from Corelib

- Unix globalization functions were using StringBuilder.  Replaced them with stackallocs, as the max sizes were all relatively small.
- Registry methods were declared to use StringBuilder, but these weren't actually used by corelib, and in fact, corefx isn't using StringBuilder with them either.  I've changed the definitions for now to use char[] instead (all the call sites are passing in null), and I'll fix up some corefx usage separately.
- Resource-related functions were using StringBuilder, and have been replaced with stackallocs, given reasonably small max buffer sizes.
- ExpandEnvironmentVariables was using a StringBuilder, but it's rewritten equivalent code in corefx is not.  For now I've copied the corefx function over to coreclr to replace the different implementation, but we can look at subsequently consolidating them into one.

* Address PR feedback


Commit migrated from dotnet/coreclr@a9b57bd
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers

@jkotas jkotas jkotas approved these changes

@tarekgh tarekgh tarekgh left review comments

@JeremyKuhne JeremyKuhne JeremyKuhne approved these changes

@eerhardt eerhardt eerhardt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@stephentoub @jkotas @JeremyKuhne @eerhardt @tarekgh