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 number parsing with negative sign #47613

Merged
merged 3 commits into from
Jan 29, 2021

Conversation

tarekgh
Copy link
Member

@tarekgh tarekgh commented Jan 29, 2021

Fixes #47524

This change allow parsing negative numbers written with the hyphen character - when using cultures with negative sign not hyphen but it is one of the alternative Unicode minus sign characters.
I have done some basic perf testing with this change and didn't notice any noticeable change.

@ghost
Copy link

ghost commented Jan 29, 2021

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

Issue Details

Fixes #47524

This change allow parsing negative numbers written with the hyphen character - when using cultures with negative sign not hyphen but it is one of the alternative Unicode minus sign characters.
I have done some basic perf testing with this change and didn't notice any noticeable change.

Author: tarekgh
Assignees: -
Labels:

area-System.Globalization

Milestone: -

@tarekgh tarekgh self-assigned this Jan 29, 2021
@tarekgh tarekgh added this to the 6.0.0 milestone Jan 29, 2021
Copy link
Member

@GrabYourPitchforks GrabYourPitchforks left a comment

Choose a reason for hiding this comment

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

Do we also need to update runtime/src/libraries/Common/src/System/Globalization/FormatProvider.Number.cs? There are some references to NumberFormatInfo.NegativeSign in that file.

Other files to inspect:

  • System/Windows/Forms/NumericUpDown.cs
  • System/Text/Json/Serialization/Converters/Value/EnumConverter.cs

(That latter file stores current culture data into a static field, which is probably a bug.)

@tarekgh
Copy link
Member Author

tarekgh commented Jan 29, 2021

@GrabYourPitchforks

Do we also need to update runtime/src/libraries/Common/src/System/Globalization/FormatProvider.Number.cs?

Yes, you are right. I'll fix this one too. I am not sure why we have parsing code in multiple places.

System/Windows/Forms/NumericUpDown.cs

This file is in WinForms repo and not runtime. I looked at it and they have keyInput.Equals(negativeSign) if they need this work correctly they can convert it to keyInput.Equals(negativeSign, StringComparison.CurrentCulture).

System/Text/Json/Serialization/Converters/Value/EnumConverter.cs

This should be fine because they are testing with the linguistic StartsWith which should work.

@tarekgh
Copy link
Member Author

tarekgh commented Jan 29, 2021

Any other comments here?

Copy link
Member

@GrabYourPitchforks GrabYourPitchforks left a comment

Choose a reason for hiding this comment

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

LGTM. Though I'd still like to see a comment on the switch statement saying "here's where we pulled this list of values from."

@GrabYourPitchforks
Copy link
Member

@tarekgh I sent an IM to you on Teams asking for clarification on something. Check that if you have a sec before merging.

@danmoseley
Copy link
Member

Do we need to create a followup issue track reducing duplication you discovered here?

@tarekgh
Copy link
Member Author

tarekgh commented Jan 29, 2021

I just discovered there some changes I didn't submit yet :-( so I am going to submit it soon.

@GrabYourPitchforks
Copy link
Member

@danmosemsft There's a tracking issue at #28657.

@tarekgh
Copy link
Member Author

tarekgh commented Jan 29, 2021

Now all changes is submitted.

@GrabYourPitchforks
Copy link
Member

Thanks for adding the comments and tests! 👍

@tarekgh tarekgh merged commit 077489f into dotnet:master Jan 29, 2021
@tarekgh tarekgh deleted the FixNegativeSignParsing branch January 29, 2021 22:08
@ghost ghost locked as resolved and limited conversation to collaborators Feb 28, 2021
Sign up for free to 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.

Swedish locale, NumberFormat.NegativeSign from ICU ignores parseLenient data from CLDR
4 participants