Skip to content

Use correct NumberStyle when parsing double and float - #59205

Merged
tannergooding merged 3 commits into
dotnet:mainfrom
7rakir:Fix-decimal-point-parsing-in-IParseable-TryParse
Sep 20, 2021
Merged

Use correct NumberStyle when parsing double and float#59205
tannergooding merged 3 commits into
dotnet:mainfrom
7rakir:Fix-decimal-point-parsing-in-IParseable-TryParse

Conversation

@7rakir

@7rakir7rakir commented Sep 16, 2021

Copy link
Copy Markdown
Contributor

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

Richard Sefr
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
@ghostghost added area-System.Numerics community-contribution Indicates that the PR has been added by a community member labels Sep 16, 2021
@ghost

ghost commented Sep 16, 2021

Copy link
Copy Markdown

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

dnfadmin commented Sep 16, 2021•
edited
Loading

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

tannergooding
tannergooding approved these changes Sep 16, 2021
View reviewed changes

@tannergoodingtannergooding 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

@tannergooding

tannergooding commented Sep 16, 2021

Copy link
Copy Markdown
Member

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

pgovind commented Sep 16, 2021

Copy link
Copy Markdown

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

@jeffhandley

jeffhandley commented Sep 16, 2021

Copy link
Copy Markdown
Member

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

tannergooding commented Sep 16, 2021

Copy link
Copy Markdown
Member

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

danmoseley commented Sep 16, 2021

Copy link
Copy Markdown
Contributor

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
Comment threadsrc/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));

@danmoseleydanmoseleySep 16, 2021

Copy link
Copy Markdown
Contributor

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

danmoseley commented Sep 16, 2021

Copy link
Copy Markdown
Contributor

Can we include tests please?

@tannergooding

tannergooding commented Sep 17, 2021

Copy link
Copy Markdown
Member

@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).

@nietrasnietras mentioned this pull request Sep 19, 2021
Closed
am11
am11 reviewed Sep 19, 2021
View reviewed changes
Comment threadMSBuild_Logs/MSBuild_pid-37116_6904092b1c9a481da960c9e28eba2a27.failure.txt Outdated
@tannergooding
tannergooding merged commit 7691aa8 into dotnet:mainSep 20, 2021
@tannergooding

tannergooding commented Sep 20, 2021

Copy link
Copy Markdown
Member

/backport to release/6.0

@github-actions

github-actionsBot commented Sep 20, 2021

Copy link
Copy Markdown
Contributor

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

@github-actionsgithub-actionsBot mentioned this pull request Sep 20, 2021
Merged
@tannergooding

tannergooding commented Sep 20, 2021

Copy link
Copy Markdown
Member

/backport to release/6.0-rc2

@github-actions

github-actionsBot commented Sep 20, 2021

Copy link
Copy Markdown
Contributor

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

@github-actionsgithub-actionsBot 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
@ghostghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jeffhandleyjeffhandleyjeffhandley approved these changes
@tannergoodingtannergoodingtannergooding approved these changes
+3 more reviewers
@am11am11am11 left review comments
@danmoseleydanmoseleydanmoseley left review comments
@pgovindpgovindpgovind approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Numericscommunity-contributionIndicates 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