Share ICU culture init for unix/win with validation#46660

Merged

tarekgh merged 1 commit intodotnet:masterfrom

am11:feature/System.Globalization/culture-names-validation

Jan 8, 2021

Member

am11 commented Jan 7, 2021

Fixes #46577

Dotnet-GitSync-Bot added the area-System.Globalization label

ghost commented Jan 7, 2021

Tagging subscribers to this area: @tarekgh, @safern, @krwq
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #46577

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

tarekgh reviewed

View reviewed changes

src/libraries/Native/Unix/System.Globalization.Native/pal_locale.c Outdated Show resolved Hide resolved

Member

tarekgh commented Jan 7, 2021

int32_t GetLocale(const UChar* localeName,

Could you look at all callers of this method to ensure the passed locale name always checked by the managed validation? just in case anyone calling this method in some private way.

Refers to: src/libraries/Native/Unix/System.Globalization.Native/pal_locale.c:30 in 387ce51. [](commit_id = 387ce51fda203b0ab5fbe9e6248489d8cc246f20, deletion_comment = False)

tarekgh reviewed

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.Unix.cs Outdated Show resolved Hide resolved

tarekgh reviewed

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.Unix.cs Outdated Show resolved Hide resolved

tarekgh reviewed

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.Unix.cs Outdated Show resolved Hide resolved

tarekgh reviewed

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.Unix.cs Outdated Show resolved Hide resolved

tarekgh reviewed

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.Unix.cs Outdated Show resolved Hide resolved

Member Author

am11 commented Jan 7, 2021 •

edited

Loading

Windows leg failures show some difference between 'ICU on Unix' and 'ICU on Windows with Kernel32 call: GetLocaleInfoEx'. I think we can look into skipping the call to GetLocaleInfoEx and unify this code, rather than omitting those cases from the unit-tests.

am11 changed the title ~~Improve culture name validation on Unix~~ Share ICU culture init for unix/win with validation

am11 marked this pull request as ready for review

January 7, 2021 12:48

stephentoub reviewed

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.Windows.cs Outdated Show resolved Hide resolved

tarekgh reviewed

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.Icu.cs Outdated Show resolved Hide resolved

safern reviewed

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.Icu.cs Outdated Show resolved Hide resolved

tarekgh reviewed

View reviewed changes

src/libraries/Native/Unix/System.Globalization.Native/pal_locale.c Outdated Show resolved Hide resolved

runfoapp bot mentioned this pull request

Testing out the new system jaredpar/runfo#71

Open

tarekgh approved these changes

View reviewed changes

Member

tarekgh left a comment

@am11 thanks a lot for your help with that.

safern approved these changes

View reviewed changes

Member

safern left a comment

Great, thanks for your help!


          Share ICU culture init for unix/win with validation

afc7918

tarekgh merged commit 805e288 into dotnet:master

am11 deleted the feature/System.Globalization/culture-names-validation branch

January 8, 2021 18:04

ghost locked as resolved and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Globalization