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

Implement IOList BIFs #362

Merged
merged 29 commits into from Jan 14, 2020
Merged

Conversation

mlwilkerson
Copy link
Contributor

@mlwilkerson mlwilkerson commented Nov 11, 2019

Resolves #153.

  • Implement and test iolist_to_binary/1
  • Implement and test iolist_size/1
  • Implement and test iolist_to_iovec/1
  • Add testing for SubBinary
  • Figure out whether to add testing for MatchContext
    This doesn't seem to be possible yet.

@mlwilkerson
Copy link
Contributor Author

I'm starting with pretty naive implementations here, to get my feet wet. This is my first Rust programming, and first exposure to both the Lumen dev environment and to iolist, so I've gotta find a handhold somewhere.

It's taken a bit of digging to find good information on the use cases for iolist vs the similar list_to_binary/1. (My initial implementations delegate to list_to_binary/1.)

I'm beginning to understand the use case of optimizing memory usage when streaming data such as when writing to a file or socket. The iolist functions can allow for more efficient list operations and memory usage. Handling improper lists is part of this use case. Helpful resources on this: here, here, here, here.

@mlwilkerson mlwilkerson changed the title WIP: implement IOList BIFs Implement IOList BIFs Nov 12, 2019
@mlwilkerson mlwilkerson marked this pull request as ready for review November 12, 2019 19:53
@mlwilkerson
Copy link
Contributor Author

I'm not sure whether adding test coverage for the binary sub-types is important. Interested to hear what a reviewer thinks about that.

@mlwilkerson
Copy link
Contributor Author

@KronicDeth do you have any feedback regarding whether it's important to test with various binaries that would result in different internal typed terms? (ProcBin, HeapBinary, SubBinary, MatchContext)

Seems like I saw something possibly related on #200 .

@KronicDeth
Copy link
Collaborator

You definitely need to test subbinaries to show that you handle non-binary ones correctly.

@mlwilkerson
Copy link
Contributor Author

I rebased onto develop and fixed up some stuff that seems to have changed.

I was not successful at constructing a MatchContext that I could pass in as a term from the test module.

I tried adding a test for ProcBin, which seemed to expose the fact that the list_to_binary/1 (to which iolist_to_binary/1 currently delegates). Opened #368 to track that and proposed #369 to resolve it. So the resolution of this PR is now blocking and that resolution of those.

@KronicDeth KronicDeth added the enhancement An enhancement to existing functionality or new functionality label Jan 14, 2020
@KronicDeth KronicDeth added this to the In 2020 milestone Jan 14, 2020
Recursion isn't safe to do in Rust the way it is in Erlang/Elixir.
Fixes binaries in iolists showing up as empty spaces like `[0 | ]` for
`[0 | ""]`.
Include proper argument name instead of using "string".
@KronicDeth KronicDeth merged commit 305e844 into GetFirefly:develop Jan 14, 2020
@KronicDeth KronicDeth mentioned this pull request Jan 15, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement to existing functionality or new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IOList BIFs
2 participants