Skip to content

Use correct NumberStyle when parsing double and float #59205

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 20, 2021
Merged

Use correct NumberStyle when parsing double and float #59205

merged 3 commits into from
Sep 20, 2021

Conversation

@7rakir
Copy link
Contributor

@7rakir 7rakir commented Sep 16, 2021

IParseable<float> and IParseable<double> would fail when parsing a decimal point number due to incorrect NumberStyles parameter.

E.g. IParseable<double>.TryParse would fail as opposed to IParseable<double>.Parse because the former explicitly states to use NumberStyles.Integer

User correct NumberStyle when parsing double and float
d6c0bdf 
IParseable<float> and IParseable<double> would fail when parsing
a decimal point number due to incorrect NumberStyles parameter.

E.g. IParseable<double>.TryParse would fail as opposed to
IParseable<double>.Parse because the former explicitly states to use
NumberStyles.Integer
@ghost ghost added area-System.Numerics community-contribution Indicates that the PR has been added by a community member labels Sep 16, 2021
@ghost
Copy link

ghost commented Sep 16, 2021

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

Issue Details

IParseable<float> and IParseable<double> would fail when parsing a decimal point number due to incorrect NumberStyles parameter.

E.g. IParseable<double>.TryParse would fail as opposed to IParseable<double>.Parse because the former explicitly states to use NumberStyles.Integer

Author: 7rakir
Assignees: -
Labels:

area-System.Numerics
Milestone: -

@dnfadmin
Copy link

dnfadmin commented Sep 16, 2021 •
edited
Loading

CLA assistant check
All CLA requirements met.

tannergooding
tannergooding approved these changes Sep 16, 2021
View reviewed changes
Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

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

LGTM

@tannergooding
Copy link
Member

tannergooding commented Sep 16, 2021

CC. @jeffhandley, can we do a bar check for this? Generic math is a preview feature, but it would be nice for this to work correctly

@pgovind
Copy link

pgovind commented Sep 16, 2021

+1 on backporting this, especially considering that it's behind a preview attribute

@jeffhandley
Copy link
Member

jeffhandley commented Sep 16, 2021

This has my support for either RC2 or GA. Customer-reported and fixed functional issue in new functionality introduced in a late preview, plus the changes are limited to the Generic Math Preview feature.

@danmoseley, what do you think?

@tannergooding
Copy link
Member

tannergooding commented Sep 16, 2021

I'll merge this PR either way after and then backport and fill in the template if Dan also thinks its fine to take.

@danmoseley
Copy link
Member

danmoseley commented Sep 16, 2021

I think we can take changes to the preview paths for a while, including this one. Assuming risk to existing codepaths is negligible. So yeah please template and PR against RC2 if you want to.

danmoseley
danmoseley reviewed Sep 16, 2021
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/Half.cs
{
if (s == null) ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
return Number.ParseHalf(s, NumberStyles.Float | NumberStyles.AllowThousands, NumberFormatInfo.GetInstance(provider));
return Number.ParseHalf(s, DefaultParseStyle, NumberFormatInfo.GetInstance(provider));
Copy link
Member

@danmoseley danmoseley Sep 16, 2021

Choose a reason for hiding this comment

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

Noting that this and above are the only changes to non preview code and they're a no-op. (It might have been nice if we used DefaultParseStyle in Float and Double or not at all, so they look the same)

@danmoseley
Copy link
Member

danmoseley commented Sep 16, 2021

Can we include tests please?

@tannergooding
Copy link
Member

tannergooding commented Sep 17, 2021

@danmoseley, I'll add some basic tests. I don't think we want to duplicate all the parsing tests as these explicit bindings are going away here shortly (they were only for .NET 6 to help separate stable vs experimental APIs).

@nietras nietras mentioned this pull request Sep 19, 2021
Closed
@tannergooding tannergooding merged commit 7691aa8 into dotnet:main Sep 20, 2021
@tannergooding
Copy link
Member

tannergooding commented Sep 20, 2021

/backport to release/6.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 20, 2021

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1254170359

@github-actions github-actions bot mentioned this pull request Sep 20, 2021
Merged
@tannergooding
Copy link
Member

tannergooding commented Sep 20, 2021

/backport to release/6.0-rc2

@github-actions
Copy link
Contributor

github-actions bot commented Sep 20, 2021

Started backporting to release/6.0-rc2: https://github.com/dotnet/runtime/actions/runs/1254861149

@github-actions github-actions bot mentioned this pull request Sep 20, 2021
Merged
@7rakir 7rakir deleted the Fix-decimal-point-parsing-in-IParseable-TryParse branch September 21, 2021 18:32
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@danmoseley danmoseley danmoseley left review comments

@jeffhandley jeffhandley jeffhandley approved these changes

@tannergooding tannergooding tannergooding approved these changes

+2 more reviewers

@am11 am11 am11 left review comments

@pgovind pgovind pgovind approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Numerics community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@7rakir @dnfadmin @tannergooding @pgovind @jeffhandley @danmoseley @am11