Skip to content

Fix System.Number.BigInteger.DivRem outputting an incorrect remainder#72115

Merged
dakersnar merged 1 commit into
dotnet:mainfrom
dakersnar:fix-72113
Jul 22, 2022
Merged

Fix System.Number.BigInteger.DivRem outputting an incorrect remainder#72115
dakersnar merged 1 commit into
dotnet:mainfrom
dakersnar:fix-72113

Conversation

@dakersnar

@dakersnar dakersnar commented Jul 13, 2022

Copy link
Copy Markdown
Contributor

Fixes #72113

@ghost ghost assigned dakersnar Jul 13, 2022
@dakersnar dakersnar requested a review from tannergooding July 13, 2022 18:25
@EgorBo

EgorBo commented Jul 13, 2022

Copy link
Copy Markdown
Member

Perhaps, worth having a unit test?

@dakersnar

Copy link
Copy Markdown
Contributor Author

@EgorBo Can do, let me try to figure out InternalsVisableTo

@stephentoub

Copy link
Copy Markdown
Member

let me try to figure out InternalsVisableTo

We try to avoid InternalsVisibleTo, for a variety of reasons, e.g. it inhibits assembly-level trimming we do.

Why does this require internals access? If there's no way to trigger this via public surface area, I think it's fine to forego a test.

@dakersnar

Copy link
Copy Markdown
Contributor Author

@stephentoub That makes sense. At the very least, once I merge the double to decimal conversion code, that will include a test that hits this code path. Do you think that's sufficient?

@stephentoub

Copy link
Copy Markdown
Member

If there's no other way to trigger this bug via public surface area, sounds fine.

@azure-pipelines

Copy link
Copy Markdown
You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@dakersnar

Copy link
Copy Markdown
Contributor Author

/azp run runtime

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ghost

ghost commented Jul 15, 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

Fixes #72113

Author: dakersnar
Assignees: dakersnar
Labels:

area-System.Numerics

Milestone: -

@dakersnar

Copy link
Copy Markdown
Contributor Author

@jeffhandley I think we discussed wanting to get this into 7.0. What does that process look like at this point?

@dakersnar dakersnar merged commit bca7f94 into dotnet:main Jul 22, 2022
@stephentoub

stephentoub commented Jul 22, 2022

Copy link
Copy Markdown
Member

I think we discussed wanting to get this into 7.0. What does that process look like at this point?

Everything going into main right now will make it into .NET 7. No additional process required (for a few more weeks).

@ghost ghost locked as resolved and limited conversation to collaborators Aug 22, 2022
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.

System.Number.BigInteger.DivRem can output an incorrect remainder

3 participants