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

Added split_once and index_of to bitarray #629

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

codemonkey76
Copy link

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.

Copy link
Member

@lpil lpil left a 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) ->
Copy link
Member

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 🙏

Copy link
Author

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>>

Copy link
Member

@lpil lpil Jun 15, 2024

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.

Copy link
Author

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

test/gleam/bit_array_test.gleam Outdated Show resolved Hide resolved
Copy link
Member

@lpil lpil left a 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
Copy link
Member

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.
Copy link
Member

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
}
Copy link
Member

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

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.

2 participants