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

Switch bigint division usage to Euclidean to match Go #723

Merged
merged 4 commits into from
Oct 1, 2020

Conversation

austinabell
Copy link
Contributor

@austinabell austinabell commented Sep 30, 2020

Summary of changes
Changes introduced in this pull request:

  • Updates all bigint divisions that can be negative to Euclidean (floored) division from truncated
    • Go uses Euclidean for bigints and truncated for integers, we can match with these changes
    • Did not change some of the bigint division that is guaranteed to be positive (truncated and floored are same for positive values). If anyone is really uncomfortable by this, I can switch others, but it means more allocations because div_floor does not allow for dividing by anything but a bigint
  • Fixes bug in miner actor VestSpec constant
    • With both changes, can uncomment the tipset conformance vector skip

Reference issue to close (if applicable)

Closes

Other information and links

Copy link
Contributor

@timvermeulen timvermeulen left a comment

Choose a reason for hiding this comment

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

This seems reasonable enough, it does improve the case for using newtypes though – it's quite possible we'll mess this up at some point in the future by incorrectly writing /. Something to think about

@austinabell
Copy link
Contributor Author

This seems reasonable enough, it does improve the case for using newtypes though – it's quite possible we'll mess this up at some point in the future by incorrectly writing /. Something to think about

I totally agree, but there is a huge amount of overhead with wrapping the bigint interface, I've done it before and there is way too many traits needed to be forwarded. I'm definitely open to this in the future, but maybe until we hit milestones we just keep in mind and be a bit careful?

@austinabell austinabell merged commit e59b7ae into main Oct 1, 2020
@austinabell austinabell deleted the austin/int/floor_div branch October 1, 2020 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Interop Interop with Lotus/specs-actors and testing Status: Needs Review VM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants