Skip to content

Fix Native Digits Support - #71045

Merged
tarekgh merged 6 commits into
dotnet:mainfrom
tarekgh:FixNativeDigitsSupport
Jun 22, 2022
Merged

Fix Native Digits Support#71045
tarekgh merged 6 commits into
dotnet:mainfrom
tarekgh:FixNativeDigitsSupport

Conversation

@tarekgh

@tarekghtarekgh commented Jun 21, 2022

Copy link
Copy Markdown
Member

We always assumed the native digits one character. This was the case for NLS. Now in ICU, there are some locales which use native digits expressed as surrogate pairs. ccp-Cakm-BD locale is a good example of that which uses the native digits "\U0001E950", "\U0001E951", "\U0001E952", "\U0001E953", "\U0001E954", "\U0001E955", "\U0001E956", "\U0001E957", "\U0001E958", "\U0001E959". This change is to allow reading the native digits correctly from ICU.

@ghost

Copy link
Copy Markdown

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

Issue Details

We always assumed the native digits always one character. This was the case for NLS. Now in ICU, there are some locales which use native digits expressed as surrogate pairs. ccp-Cakm-BD locale is a good example of that which uses the native digits "\U0001E950", "\U0001E951", "\U0001E952", "\U0001E953", "\U0001E954", "\U0001E955", "\U0001E956", "\U0001E957", "\U0001E958", "\U0001E959". This change is to allow reading the native digits correctly from ICU.

Author:tarekgh
Assignees:-
Labels:

area-System.Globalization

Milestone:-

@tarekghtarekgh added this to the 7.0.0 milestone Jun 21, 2022
@tarekgh

Copy link
Copy Markdown
MemberAuthor

CC @eerhardt@maryamariyan if you can help review it.

@danmoseley

Copy link
Copy Markdown
Contributor

Does this affect treatment of \d in a regex pattern?

@stephentoub

Copy link
Copy Markdown
Member

Does this affect treatment of \d in a regex pattern?

No

Comment threadsrc/native/libs/System.Globalization.Native/pal_localeStringData.c Outdated
Comment threadsrc/native/libs/System.Globalization.Native/pal_localeStringData.c Outdated
Comment threadsrc/native/libs/System.Globalization.Native/pal_localeStringData.c Outdated
Comment threadsrc/native/libs/System.Globalization.Native/pal_localeStringData.c Outdated
@tarekgh

Copy link
Copy Markdown
MemberAuthor

@eerhardt@stephentoub do you have any more comments?

@eerhardteerhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks @tarekgh!

@tarekgh
tarekgh merged commit 315e931 into dotnet:mainJun 22, 2022
@tarekgh
tarekgh deleted the FixNativeDigitsSupport branch June 22, 2022 23:00

} while (ffffPos < digits.Length && index < 10);

Debug.Assert(index >= 10, $"Couldn't read native digits for '{_sWindowsName}' successfully.");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be index == 10? result is exactly 10 items long, so if we tried to do result[10], it would have thrown.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes, we can do that. I am not sure if this is worth another PR though.

@ghostghost locked as resolved and limited conversation to collaborators Jul 23, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@tarekgh@danmoseley@stephentoub@eerhardt