Skip to content

Conversation

@wzchua
Copy link
Contributor

@wzchua wzchua commented Jun 13, 2021

Fixes #27358

The issue was due to the 2's complement transformation when the big integer representation was [0xFFFFFFFF, 0xFFFFFFFF].
That becomes [0, 1]. and when you shift it by 32 bits you get [0]. so when you transform it back you still get 0 as the value

@ghost ghost added the area-System.Numerics label Jun 13, 2021
@ghost
Copy link

ghost commented Jun 13, 2021

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

Issue Details

Fixes #27358

The issue was due to the 2's complement transformation when the big integer representation was [0xFFFFFFFF, 0xFFFFFFFF].
That becomes [0, 1]. and when you shift it by 32 bits you get [0]. so when you transform it back you still get 0 as the value

Author: wzchua
Assignees: -
Labels:

area-System.Numerics

Milestone: -

@tannergooding
Copy link
Member

This LGTM. However, could you please add a test covering the failing scenario?

CC. @pgovind for secondary review.

@wzchua
Copy link
Contributor Author

wzchua commented Jun 18, 2021

The test is already there.

@tannergooding
Copy link
Member

The test is already there.

So it is, I completely missed that file for some reason. Sorry for the confusion.

@terrajobst terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021
@jeffhandley
Copy link
Member

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jeffhandley
Copy link
Member

As far as I can tell, this should be ready to merge once we get a green run from CI. I've restarted the run.

@tannergooding This PR is assigned to you for follow-up before the RC1 snap.

Copy link

@pgovind pgovind left a comment

Choose a reason for hiding this comment

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

LGTM

@jeffhandley
Copy link
Member

Test failure is related to #56190.

@jeffhandley jeffhandley merged commit 4ee01db into dotnet:main Jul 24, 2021
@jeffhandley
Copy link
Member

Thanks for the contribution, @wzchua!

@wzchua wzchua deleted the fix/biginteger-negative-32bit-right-shift branch July 24, 2021 04:38
@ghost ghost locked as resolved and limited conversation to collaborators Aug 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Numerics community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Incorrect result for 32-bit right shift of specific negative values in BigInteger

5 participants