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

Signed int keys order #582

Merged
merged 19 commits into from
Dec 17, 2021
Merged

Signed int keys order #582

merged 19 commits into from
Dec 17, 2021

Conversation

maurolacy
Copy link
Contributor

@maurolacy maurolacy commented Dec 16, 2021

Closes #489.

There are a number of different ways to do the signed int key modification (a number of different impls in the commits). Not sure what is the best one; opted for doing it without mutable slices, and without branching.

I guess the only way to know for sure would be by benchmarking them. Will create an issue for it, and we can do in the future.

Copy link
Contributor

@ueco-jb ueco-jb left a comment

Choose a reason for hiding this comment

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

Good to see that this idea actually worked just fine.

packages/storage-plus/src/keys.rs Outdated Show resolved Hide resolved
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

looks good.
I would clean up the test cases to be more explicit rather than just contain the same math being tested

also, let's add test case that -321.to_cw_bytes() < 0.to_cw_bytes() < 652.to_cw_bytes()

packages/storage-plus/src/de.rs Show resolved Hide resolved
packages/storage-plus/src/de.rs Outdated Show resolved Hide resolved
packages/storage-plus/src/de.rs Outdated Show resolved Hide resolved
packages/storage-plus/src/de.rs Show resolved Hide resolved
packages/storage-plus/src/keys.rs Show resolved Hide resolved
packages/storage-plus/src/int_key.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@ueco-jb ueco-jb left a comment

Choose a reason for hiding this comment

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

lgtm
Personally I'd replace couple big numbers with constants for better transparency, but overall it's fine.

@maurolacy maurolacy merged commit 6ae0f84 into main Dec 17, 2021
@maurolacy maurolacy deleted the 489-signed-int-keys branch December 17, 2021 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect I32Key Index Ordering
3 participants