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

Binary key support + key trait #49

Merged
merged 11 commits into from Jan 21, 2020
Merged

Binary key support + key trait #49

merged 11 commits into from Jan 21, 2020

Conversation

gballet
Copy link
Owner

@gballet gballet commented Dec 6, 2019

This is preparation work for the support of binary trees.

  • It introduces a Key trait that will have to be implemented by all objects that can be used as keys;
  • It introduces BinaryKey that implements Key and represents a bit-wise string;
  • It rewrites ByteKey and NibbleKey as an implementation of Key; and
  • it moves ByteKey, NibbleKey and BinaryKey to their own modules.

@gballet gballet requested a review from s1na December 17, 2019 17:59
@gballet gballet mentioned this pull request Dec 19, 2019
2 tasks
/// | start end
/// ```
#[derive(Debug, PartialEq, Clone)]
pub struct BinaryKey(pub Vec<u8>, pub usize, pub usize); // (key data, offset in first byte, offset in last byte)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion: instead of exposing the underlying byte-based structure, let the calling code use general bit offsets. By that I mean the first bit from left has offset 0 and if the key length is 16, the last bit (rightmost) has offset 15. This feels more intuitive for me.
Although I can see when you repeatedly want to slice the key, then those offsets change.

@gballet
Copy link
Owner Author

gballet commented Jan 20, 2020

Based on @s1na's remarks, I have decided to go another route, outlined in #55. Closing this PR.

@gballet gballet closed this Jan 20, 2020
@gballet
Copy link
Owner Author

gballet commented Jan 21, 2020

So after considering the iterator route for some time, and in spite of being convinced that this is the best way in the long run, I also realized it was going to be fairly heavy work, and it's not currently necessary if it's just about not making a couple fields public. I will go ahead with this one in order to resume work on binary trees, and will fix that for the 2.x release.

@gballet gballet reopened this Jan 21, 2020
@gballet gballet merged commit 6fa756f into master Jan 21, 2020
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.

None yet

2 participants