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

bytes_to_field_elements should accept and return an iterator #300

Closed
ggutoski opened this issue Jun 6, 2023 · 2 comments · Fixed by #381
Closed

bytes_to_field_elements should accept and return an iterator #300

ggutoski opened this issue Jun 6, 2023 · 2 comments · Fixed by #381
Assignees
Labels
tech debt Technical debt. We all pay eventually!

Comments

@ggutoski
Copy link
Contributor

ggutoski commented Jun 6, 2023

Problem

Originally posted by @ggutoski in #299 (comment)

Currently bytes_to_field_elements takes AsRef<[u8]> as input and returns Vec<F> as output. This can lead to unnecessary mem copy or heap alloc if the caller doesn't already have a AsRef<[u8]> or wants the output in some form other than Vec<F>.

Example: here the caller actually wants a Vec<Vec<F>>, and needs to copy/allocate a whole 2D vec for this purpose.

https://github.com/EspressoSystems/hotshot-primitives/blob/52b2f713eea897778eb8a73f401c9b1b4a2dde19/src/vid/advz.rs#L158-L159

Solution

bytes_to_field_elements should accept IntoIterator instead of AsRef<[u8]> and should return an iterator over F instead of Vec<F>.

We should do the same for bytes_from_field_elements, though that will be more complicated to implement.

This solution is an example of the iterator adaptor pattern:

@ggutoski ggutoski self-assigned this Jun 6, 2023
@ggutoski ggutoski added low-priority tech debt Technical debt. We all pay eventually! labels Jun 6, 2023
@amit0365
Copy link

Hello I am planning to work on this. Are you close to implementing the solution @ggutoski ?

@ggutoski
Copy link
Contributor Author

Hello I am planning to work on this. Are you close to implementing the solution @ggutoski ?

Hello @amit0365 . I have spent no time on this issue. Feel free to work on it! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech debt Technical debt. We all pay eventually!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants