-
-
Notifications
You must be signed in to change notification settings - Fork 182
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
Added split_once and index_of to bitarray #629
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely work! Some small notes inline
_ -> bit_array_find_needle(Haystack, Needle, Pos + 1, HaystackSize, NeedleSize) | ||
end. | ||
|
||
bit_array_split_once(Haystack, Needle) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the built in split function please 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The built in function doesn't seem to allow splitting along non-byte aligned boundaries.
e.g. splitting
<<1, 2, 3, 4, 5>>, with <<3:2>> returns {error, nil}
Current implementation correctly splits it as:
<<1, 2, 0:6>>, <<4, 5>>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine 👍 Performance is the priority here, and I'm not aware of any binary protocol that will use non-byte aligned data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, i removed the associated tests, and switched to using erlang's built in binary:split
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Could you update the changelog also please
/// | ||
@external(erlang, "gleam_stdlib", "bit_array_index_of") | ||
@external(javascript, "../gleam_stdlib.mjs", "bit_array_index_of") | ||
pub fn index_of(haystack: BitArray, needle: BitArray) -> Int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this please 🙏 The ticket was only for split_once
.
@external(javascript, "../gleam_stdlib.mjs", "bit_array_index_of") | ||
pub fn index_of(haystack: BitArray, needle: BitArray) -> Int | ||
|
||
// error is returned if not found. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems to have been misplaced
<<"Hello, World":utf8>> | ||
|> bit_array.split_once(<<"Joe":utf8>>) | ||
|> should.be_error | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you include tests for non-byte aligned bit arrays please
I messed up the last pull request, and had like over 100 test failures. Not sure what happened, so here is attempt #2.
I just have to get the erlang non-byte aligned working and tested.