Skip to content

Fix Exp10M1(TSelf) and Round(TSelf, MidpointRounding) #75063

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 8 commits into from
Sep 8, 2022
Merged

Fix Exp10M1(TSelf) and Round(TSelf, MidpointRounding) #75063

merged 8 commits into from
Sep 8, 2022

Conversation

@blokyk
Copy link
Contributor

@blokyk blokyk commented Sep 3, 2022 •
edited
Loading

IExponentialFunctions.Exp10M1 had a typo that caused it to go into infinite recursion, and IFloatingPoint.Round straight up just ignored it's mode argument, most likely just a typo again.

Fixes #75059

@ghost ghost added area-System.Numerics community-contribution Indicates that the PR has been added by a community member labels Sep 3, 2022
@ghost
Copy link

ghost commented Sep 3, 2022

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

Issue Details

IExponentialFunctions.Exp10M1 had a type that caused it to go into infinite recursion, and IFloatingPoint.Round straight up just ignored it's mode argument, most likely just a typo again.

Fixes #75059

Author: Blokyk
Assignees: -
Labels:

area-System.Numerics
Milestone: -

@blokyk
Copy link
Contributor Author

blokyk commented Sep 3, 2022

I apologize for the lack of tests -- I'm currently experiencing connectivity issues, which makes cloning and building the runtime a bit tricky. I'm writing some right now for both of the functions that I fixed, but if needed I can try to add more while I wait for my internet to come back. Sorry for submitting this PR without the tests first

@blokyk blokyk marked this pull request as draft September 4, 2022 00:12
@blokyk blokyk marked this pull request as ready for review September 4, 2022 14:02
@tannergooding
Copy link
Member

I apologize for the lack of tests -- I'm currently experiencing connectivity issues, which makes cloning and building the runtime a bit tricky. I'm writing some right now for both of the functions that I fixed, but if needed I can try to add more while I wait for my internet to come back. Sorry for submitting this PR without the tests first

Absolutely no worries. We'd much rather have a fix up than not at all. Someone else can always pick it up and add tests if necessary/etc 😄

tannergooding
tannergooding approved these changes Sep 6, 2022
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.

Changes LGTM. Just a few minor nits to use float instead of using Single

dakersnar
dakersnar approved these changes Sep 6, 2022
View reviewed changes
Copy link
Contributor

@dakersnar dakersnar left a comment

Choose a reason for hiding this comment

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

LGTM, and thanks for submitting unit tests too.

@tannergooding tannergooding merged commit d12a954 into dotnet:main Sep 8, 2022
@tannergooding
Copy link
Member

/backport to release/7.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2022

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3016956207

@github-actions github-actions bot mentioned this pull request Sep 8, 2022
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Oct 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@tannergooding tannergooding tannergooding approved these changes

+1 more reviewer

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

Wrong DIM for Exp10M1(TSelf) and Round(TSelf, MidpointRounding)

3 participants

@blokyk @tannergooding @dakersnar