Skip to content

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

Merged
tannergooding merged 3 commits into
dotnet:mainfrom
tannergooding:fix-75416
Sep 13, 2022
Merged

Simplify and fix the Int128 *, /, and % logic#75470
tannergooding merged 3 commits into
dotnet:mainfrom
tannergooding:fix-75416

Conversation

@tannergooding

@tannergoodingtannergooding commented Sep 12, 2022

Copy link
Copy Markdown
Member

This resolves#75416

@ghost

ghost commented Sep 12, 2022

Copy link
Copy Markdown

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.

tfenise
tfenise suggested changes Sep 12, 2022
View reviewed changes

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

I think there are some bugs.

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Int128.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Int128.cs Outdated
@tannergooding@tfenise
Apply suggestions from code review
da89241
Co-authored-by: tfenise <tfenise@live.com>
@ghost

ghost commented Sep 12, 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

This resolves #75416

Author:tannergooding
Assignees:tannergooding
Labels:

area-System.Numerics

Milestone:-

dakersnar
dakersnar approved these changes Sep 12, 2022
View reviewed changes

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

Added some comments, once those are addressed LGTM

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Int128.cs
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Int128.cs
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Int128.cs
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Int128.cs
Int128 upper = BigMul(left, right, out Int128 lower);

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

@dakersnardakersnarSep 12, 2022

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.

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

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Int128.cs
@tannergoodingtannergooding mentioned this pull request Sep 13, 2022
Closed
@tannergooding

tannergooding commented Sep 13, 2022

Copy link
Copy Markdown
MemberAuthor

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

@tannergooding
tannergooding merged commit 721e78a into dotnet:mainSep 13, 2022
@tannergooding

tannergooding commented Sep 13, 2022

Copy link
Copy Markdown
MemberAuthor

/backport to release/7.0

@github-actions

github-actionsBot commented Sep 13, 2022

Copy link
Copy Markdown
Contributor

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

@github-actionsgithub-actionsBot mentioned this pull request Sep 13, 2022
Merged
@SamMonoRTSamMonoRT mentioned this pull request Sep 14, 2022
Closed
@ghostghost 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 freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@tfenisetfenisetfenise requested changes
@dakersnardakersnardakersnar approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@tannergoodingtannergooding

Labels

area-System.Numerics

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bugs: System.Int128 operator * and % at overflow

4 participants

@tannergooding@tfenise@dakersnar@danmoseley