Skip to content

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

Merged
tannergooding merged 8 commits into
dotnet:mainfrom
blokyk:fix-round-dim
Sep 8, 2022
Merged

Fix Exp10M1(TSelf) and Round(TSelf, MidpointRounding)#75063
tannergooding merged 8 commits into
dotnet:mainfrom
blokyk:fix-round-dim

Conversation

@blokyk

@blokyk blokyk commented Sep 3, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

ghost commented Sep 3, 2022

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

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

blokyk commented Sep 3, 2022

Copy link
Copy Markdown
Contributor Author

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

tannergooding commented Sep 6, 2022

Copy link
Copy Markdown
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 reviewed Sep 6, 2022
View reviewed changes
Comment thread src/libraries/System.Runtime/tests/System/Numerics/GenericMathDimHelpers.cs Outdated
tannergooding
tannergooding reviewed Sep 6, 2022
View reviewed changes
Comment thread src/libraries/System.Runtime/tests/System/Numerics/GenericMathDimHelpers.cs Outdated
tannergooding
tannergooding approved these changes Sep 6, 2022
View reviewed changes

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

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

Comment thread src/libraries/System.Runtime/tests/System/Numerics/GenericMathDimHelpers.cs Outdated
Comment thread src/libraries/System.Runtime/tests/System/Numerics/IExponentialFunctionsTests.cs Outdated
Comment thread src/libraries/System.Runtime/tests/System/Numerics/IFloatingPointTests.cs Outdated
Comment thread src/libraries/System.Runtime/tests/System/Numerics/IFloatingPointTests.cs Outdated
Comment thread src/libraries/System.Runtime/tests/System/Numerics/IFloatingPointTests.cs Outdated
Comment thread src/libraries/System.Runtime/tests/System/Numerics/IFloatingPointTests.cs Outdated
Comment thread src/libraries/System.Runtime/tests/System/Numerics/IFloatingPointTests.cs Outdated
dakersnar
dakersnar approved these changes Sep 6, 2022
View reviewed changes

@dakersnar dakersnar left a comment

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.

LGTM, and thanks for submitting unit tests too.

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

tannergooding commented Sep 8, 2022

Copy link
Copy Markdown
Member

/backport to release/7.0

@github-actions

github-actions Bot commented Sep 8, 2022

Copy link
Copy Markdown
Contributor

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