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.

Sign up for GitHub

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

Jump to bottom

Add take_all #142

Closed
wants to merge 2 commits into from
Closed

Add take_all #142

wants to merge 2 commits into from

Conversation

bikallem
Copy link
Contributor

@bikallem bikallem commented Jan 12, 2022 •
edited

Draft: This build on top of pr #141 and adds to_seq function to convert Stream items to Seq.t.

Perhaps this should to_list instead and return a list?

talex5
talex5 requested changes Jan 13, 2022
View reviewed changes
Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

Yes, this should return a list. Otherwise, you need to change the locking to lock on every access, not just the first one.

I suggest the name take_all for this behaviour of removing everything. to_list sounds like it shouldn't change the original.

@bikallem bikallem changed the title Add to_seq Add take_all Jan 13, 2022
@bikallem bikallem requested a review from talex5 January 13, 2022 11:24
@bikallem
Copy link
Contributor Author

bikallem commented Jan 13, 2022 •
edited

Note: the loop function uses TRMC attribute so it allocates less and I believe it is more performant than a tail rec version. TRMC is available in the mainline OCaml 5.0.
ocaml/ocaml#9760
ocaml/ocaml#10740

Should we also add map, iter and fold? I am on the fence on this issue as these can be defined outside the lib using take_nonblocking since accessing each element needs a lock which take_nonblocking provides. Perhaps for convenience we could include these functions?

talex5 and others added 2 commits January 13, 2022 13:35
@talex5 @bikallem
Minor updates to README
d1d3661 
- Clarify switch behaviour.
- Mention `Lwt_eio`.
- Note that Eio is getting more mature now.
@bikallem
lib_eio: add take_all function
36bed7b
@bikallem bikallem mentioned this pull request Jan 13, 2022
Closed
@bikallem
Copy link
Contributor Author

closing this in favour of #146

@bikallem bikallem closed this Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@talex5 talex5 Awaiting requested review from talex5

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@bikallem @talex5