NotificationsYou must be signed in to change notification settings
Fork5.1k
Star16.6k

Use u8 string in CultureNames#71417

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

stephentoub merged 3 commits intodotnet:mainfromam11:feature/u8-strings

Jul 9, 2022

Merged

Use u8 string in CultureNames#71417

stephentoub merged 3 commits intodotnet:mainfromam11:feature/u8-strings

Jul 9, 2022

Conversation

Copy link

Member

am11 commentedJun 29, 2022•
edited
Loading

~~First commit updates toolset version and applies@AaronRobinsonMSFT's lifetime patch fromdotnet/installer#14029.~~

Second commit uses u8 string for CultureNames, and updates its generator. File size ofIcuLocaleData.cs goes from 277740 to 227081 (bytes).

ghost added community-contribution

Indicates that the PR has been added by a community member

area-System.Globalization labels

Jun 29, 2022

Copy link

ghost commentedJun 29, 2022

Tagging subscribers to this area: @dotnet/area-system-globalization
See info inarea-owners.md if you want to be subscribed.

Issue Details

First commit updates toolset version and applies@AaronRobinsonMSFT's lifetime patch fromdotnet/installer#14029.

Second commit uses u8 string for CultureNames, and updates its generator. File size ofIcuLocaleData.cs goes from 277740 to 247866 (bytes).

Author:	am11
Assignees:	-
Labels:	`area-System.Globalization`,`community-contribution`
Milestone:	-

am11 requested a review fromstephentoub

June 29, 2022 04:07

am11 changed the title~~Use u8 in CultureNames~~Use u8 string in CultureNames

Jun 29, 2022

Copy link

Member

AaronRobinsonMSFT commentedJun 29, 2022

@am11 Those changes are really a point in time and shouldn't be checked into main. Moving to a new compiler withref field support should be paired with the merging in ofhttps://github.com/dotnet/runtime/tree/feature/csharp_ref_fields.

danmoseley reviewed

Jun 29, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/IcuLocaleData.csShow resolvedHide resolved

Copy link

MemberAuthor

am11 commentedJun 29, 2022

@AaronRobinsonMSFT, could you please rebase that branch currentmain? I can then merge it here.

Copy link

Member

stephentoub commentedJun 29, 2022

Let's please wait to do this PR until Aaron's changes that bring in a new compiler have all landed. There's no rush on this PR as it's purely maintainability. Thanks.

stephentoub marked this pull request as draft

June 29, 2022 10:58

Copy link

Member

AaronRobinsonMSFT commentedJun 29, 2022

@AaronRobinsonMSFT, could you please rebase that branch currentmain? I can then merge it here.

I should be able to get that in a PR today or tomorrow.

tarekgh added this to the7.0.0 milestone

Jun 29, 2022

am11 force-pushed thefeature/u8-strings branch from42170fe to74f14c7Compare

July 3, 2022 12:53

Copy link

MemberAuthor

am11 commentedJul 3, 2022•
edited
Loading

Compiler is crashing with Stack Overflow (on Windows and Applie's OSes):

/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error: Stack overflow. [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error: Repeat 339 times: [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error: -------------------------------- [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error:    at Microsoft.CodeAnalysis.CSharp.Binder.CheckValEscape(Microsoft.CodeAnalysis.SyntaxNode, Microsoft.CodeAnalysis.CSharp.BoundExpression, UInt32, UInt32, Boolean, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag) [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error: -------------------------------- [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error:    at Microsoft.CodeAnalysis.CSharp.Binder.ValidateEscape(Microsoft.CodeAnalysis.CSharp.BoundExpression, UInt32, Boolean, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag) [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error:    at Microsoft.CodeAnalysis.CSharp.Binder.<BindExpressionBodyAsBlock>g__bindExpressionBodyAsBlockInternal|960_0(Microsoft.CodeAnalysis.CSharp.Syntax.ArrowExpressionClauseSyntax, Microsoft.CodeAnalysis.CSharp.Binder, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag) [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error:    at Microsoft.CodeAnalysis.CSharp.Binder.BindExpressionBodyAsBlock(Microsoft.CodeAnalysis.CSharp.Syntax.ArrowExpressionClauseSyntax, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag) [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error:    at Microsoft.CodeAnalysis.CSharp.Binder.BindMethodBody(Microsoft.CodeAnalysis.CSharp.CSharpSyntaxNode, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag) [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error:    at Microsoft.CodeAnalysis.CSharp.MethodCompiler.BindMethodBody(Microsoft.CodeAnalysis.CSharp.Symbols.MethodSymbol, Microsoft.CodeAnalysis.CSharp.TypeCompilationState, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag, Boolean, Microsoft.CodeAnalysis.CSharp.BoundNode, Boolean, Microsoft.CodeAnalysis.CSharp.ImportChain ByRef, Boolean ByRef, Boolean ByRef, InitialState ByRef) [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error:    at Microsoft.CodeAnalysis.CSharp.MethodCompiler.CompileMethod(Microsoft.CodeAnalysis.CSharp.Symbols.MethodSymbol, Int32, ProcessedFieldInitializers ByRef, Microsoft.CodeAnalysis.CSharp.SynthesizedSubmissionFields, Microsoft.CodeAnalysis.CSharp.TypeCompilationState) [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error:    at Microsoft.CodeAnalysis.CSharp.MethodCompiler.CompileNamedType(Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol) [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error:    atMicrosoft.CodeAnalysis.CSharp.MethodCompiler+<>c__DisplayClass25_0.<CompileNamedTypeAsync>b__0() [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error:    atRoslyn.Utilities.UICultureUtilities+<>c__DisplayClass5_0.<WithCurrentUICulture>b__0() [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error:    at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(System.Threading.Thread, System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object) [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error:    at System.Threading.Tasks.Task.ExecuteWithThreadLocal(System.Threading.Tasks.Task ByRef, System.Threading.Thread) [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error:    atSystem.Threading.ThreadPoolWorkQueue.Dispatch() [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error:    atSystem.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart() [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error:    atSystem.Threading.Thread.StartCallback() [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]

@AlekseyTs, is it a known issue?

Copy link

MemberAuthor

am11 commentedJul 3, 2022

@AlekseyTs, is it a known issue?

Reporteddotnet/roslyn#62361 with additional info in case it wasn't.

jcouv mentioned this pull request

Jul 7, 2022

Stackoverflow with u8 literal concatenation on macOSdotnet/roslyn#62361

Closed

Copy link

Member

jcouv commentedJul 8, 2022

Reporteddotnet/roslyn#62361 with additional info in case it wasn't.

The roslyn issue was fixed. Updated roslyn packages are usually available within a day on the internal feeds (dotnet-tools) and on the public feeds once per preview.

am11and others added2 commits

July 8, 2022 12:15

Use u8 in CultureNames

b8dfff7

Update compiler version for u8 bug fix

89d9151

stephentoub force-pushed thefeature/u8-strings branch from74f14c7 to89d9151Compare

July 8, 2022 16:26

stephentoub marked this pull request as ready for review

July 8, 2022 16:27

stephentoub approved these changes

Jul 8, 2022

View reviewed changes

Copy link

Member

stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link

Member

tarekgh commentedJul 8, 2022

Looks there still a problem?

https://dev.azure.com/dnceng/public/_build/results?buildId=1868478&view=logs&j=c43e5161-e889-5244-7e5f-665cb038cd53&t=f5c3b7e6-0b90-53cc-c065-66aca8370440

Copy link

Member

stephentoub commentedJul 8, 2022

Looks there still a problem?

Yup.@jcouv do you know what min version has the fix?

Copy link

MemberAuthor

am11 commentedJul 8, 2022

I think we would need to wait for the next v4.4.0-x to pick up the change. (the subsequent automation PR afterdotnet/roslyn#62492 is merged or abandoned).