Skip to content
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.

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

Simplify and fix the Int128 *, /, and % logic #75470

Merged
merged 3 commits into from
Sep 13, 2022

Conversation

tannergooding
Copy link
Member

This resolves #75416

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost assigned tannergooding Sep 12, 2022
Copy link
Contributor

@tfenise tfenise left a comment

Choose a reason for hiding this comment

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

I think there are some bugs.

src/libraries/System.Private.CoreLib/src/System/Int128.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/Int128.cs Outdated Show resolved Hide resolved
Co-authored-by: tfenise <tfenise@live.com>
@ghost
Copy link

ghost commented Sep 12, 2022

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

Issue Details

This resolves #75416

Author: tannergooding
Assignees: tannergooding
Labels:

area-System.Numerics

Milestone: -

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.

Added some comments, once those are addressed LGTM


if (IsNegative(right))
if (((upper != 0) || (lower < 0)) && ((~upper != 0) || (lower >= 0)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Something about this logic is hard to parse but I can't figure out a better way to structure it.

@tannergooding
Copy link
Member Author

Logged #75566 for the Mono llvmaot failure, which is unrelated.

@tannergooding tannergooding merged commit 721e78a into dotnet:main Sep 13, 2022
@tannergooding
Copy link
Member Author

/backport to release/7.0

@github-actions
Copy link
Contributor

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

@ghost ghost locked as resolved and limited conversation to collaborators Oct 14, 2022
@tannergooding tannergooding deleted the fix-75416 branch November 11, 2022 15:11
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.

bugs: System.Int128 operator * and % at overflow
4 participants