-
Notifications
You must be signed in to change notification settings - Fork 73
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
Add Flow.read_all #596
Add Flow.read_all #596
Conversation
e61e637
to
ea8e232
Compare
lib_eio/flow.ml
Outdated
@@ -161,6 +161,12 @@ let buffer_sink = | |||
let ops = Pi.sink (module Buffer_sink) in | |||
fun b -> Resource.T (b, ops) | |||
|
|||
let load (t : _ source) = | |||
let b = Buffer.create 4096 in |
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.
Would it make sense to have the buffer size be configurable through an optional parameter? ?(size = 4096)
?
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.
Possibly. I debated whether to do it or not. Ultimately I went with no because Buffer_sink.copy
just 20 lines above also doesn't. Plus I figured that any situation where the buffer size would have an impact would not use a function that loads everything into a single large string; it would process the file in small chunks instead. It made sense to me to keep this "beginner function" as simple as possible.
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.
Having a function like this might be good. Going via Buffer
probably isn't a good idea (because then we read into a cstruct, copy it into the buffer, then copy it out again for the final string).
It can be done at the moment with e.g.
let read_all flow =
Eio.Buf_read.(parse_exn take_all) flow ~max_size:max_int
Having this in Flow might be more discoverable, but requires a little bit of extra work to avoid a cyclic dependency (because Buf_read
depends on Flow
).
I used Buffer to avoid the dependency issue since this function isn't meant to be ultra performant. I'll give it another shot this weekend and see what I can do. |
ea8e232
to
a18d2e3
Compare
@talex5 I rewrote it using Edit: I'm not convinced the performance is worth the extra complexity if this function is not intended as more than a debugging or discovery or small scale tool. |
A simpler way might be to add it in eio.ml, e.g. module Buf_read = Buf_read
module Flow = struct
include Flow
let read_all = ...
end |
Great idea, I like that a lot better. EDIT: done. Much better 🎉 |
f940408
to
9d62a6e
Compare
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.
Looks good - I just made some minor changes to the doc-strings.
CHANGES: New features / API changes: - Add `Flow.read_all` (@SGrondin ocaml-multicore/eio#596, reviewed by @talex5 @rbjorklin). - Add `Path.stat` (@patricoferris @talex5 @avsm ocaml-multicore/eio#617 ocaml-multicore/eio#618 ocaml-multicore/eio#624 ocaml-multicore/eio#620, reviewed by @SGrondin). - Add `Path.rmtree` (@talex5 ocaml-multicore/eio#627 ocaml-multicore/eio#628, reviewed by @SGrondin). - Add `Path.mkdirs` and `Path.split` (@patricoferris @talex5 ocaml-multicore/eio#625). - Add `Eio.File.{seek,sync,truncate}` (@talex5 ocaml-multicore/eio#626). - Add `Eio.Path.{kind,is_file,is_directory}` (@patricoferris @talex5 ocaml-multicore/eio#623, reviewed by @avsm). - Switch from CTF to OCaml 5.1 runtime events (@TheLortex @patricoferris @talex5 ocaml-multicore/eio#634 ocaml-multicore/eio#635, reviewed by @avsm). This is a minimal initial version. Documentation: - Document `File.Stat` record fields (@avsm @talex5 ocaml-multicore/eio#621). - Update README section about `env` (@talex5 ocaml-multicore/eio#614, reported by @jonsterling). Build and test changes: - Add `File.stat` benchmark (@talex5 ocaml-multicore/eio#616). - Add `Path.stat` benchmark (@patricoferris @talex5 ocaml-multicore/eio#630). - eio_linux: mark as only available on Linux (@talex5 ocaml-multicore/eio#629). - Make MDX tests idempotent (@SGrondin ocaml-multicore/eio#601, reviewed by @talex5). - Allow trailing whitespace in CHANGES.md (@talex5 ocaml-multicore/eio#632). - Update minimum OCaml version to 5.1 (@talex5 ocaml-multicore/eio#631). - Generate prototypes for C stubs from ml files (@talex5 ocaml-multicore/eio#615). - Don't try to compile uring support on centos 7 (@talex5 ocaml-multicore/eio#638, reported by @zenfey).
CHANGES: New features / API changes: - Add `Flow.read_all` (@SGrondin ocaml-multicore/eio#596, reviewed by @talex5 @rbjorklin). - Add `Path.stat` (@patricoferris @talex5 @avsm ocaml-multicore/eio#617 ocaml-multicore/eio#618 ocaml-multicore/eio#624 ocaml-multicore/eio#620, reviewed by @SGrondin). - Add `Path.rmtree` (@talex5 ocaml-multicore/eio#627 ocaml-multicore/eio#628, reviewed by @SGrondin). - Add `Path.mkdirs` and `Path.split` (@patricoferris @talex5 ocaml-multicore/eio#625). - Add `Eio.File.{seek,sync,truncate}` (@talex5 ocaml-multicore/eio#626). - Add `Eio.Path.{kind,is_file,is_directory}` (@patricoferris @talex5 ocaml-multicore/eio#623, reviewed by @avsm). - Switch from CTF to OCaml 5.1 runtime events (@TheLortex @patricoferris @talex5 ocaml-multicore/eio#634 ocaml-multicore/eio#635, reviewed by @avsm). This is a minimal initial version. Documentation: - Document `File.Stat` record fields (@avsm @talex5 ocaml-multicore/eio#621). - Update README section about `env` (@talex5 ocaml-multicore/eio#614, reported by @jonsterling). Build and test changes: - Add `File.stat` benchmark (@talex5 ocaml-multicore/eio#616). - Add `Path.stat` benchmark (@patricoferris @talex5 ocaml-multicore/eio#630). - eio_linux: mark as only available on Linux (@talex5 ocaml-multicore/eio#629). - Make MDX tests idempotent (@SGrondin ocaml-multicore/eio#601, reviewed by @talex5). - Allow trailing whitespace in CHANGES.md (@talex5 ocaml-multicore/eio#632). - Update minimum OCaml version to 5.1 (@talex5 ocaml-multicore/eio#631). - Generate prototypes for C stubs from ml files (@talex5 ocaml-multicore/eio#615). - Don't try to compile uring support on centos 7 (@talex5 ocaml-multicore/eio#638, reported by @zenfey).
CHANGES: New features / API changes: - Add `Flow.read_all` (@SGrondin ocaml-multicore/eio#596, reviewed by @talex5 @rbjorklin). - Add `Path.stat` (@patricoferris @talex5 @avsm ocaml-multicore/eio#617 ocaml-multicore/eio#618 ocaml-multicore/eio#624 ocaml-multicore/eio#620, reviewed by @SGrondin). - Add `Path.rmtree` (@talex5 ocaml-multicore/eio#627 ocaml-multicore/eio#628, reviewed by @SGrondin). - Add `Path.mkdirs` and `Path.split` (@patricoferris @talex5 ocaml-multicore/eio#625). - Add `Eio.File.{seek,sync,truncate}` (@talex5 ocaml-multicore/eio#626). - Add `Eio.Path.{kind,is_file,is_directory}` (@patricoferris @talex5 ocaml-multicore/eio#623, reviewed by @avsm). - Switch from CTF to OCaml 5.1 runtime events (@TheLortex @patricoferris @talex5 ocaml-multicore/eio#634 ocaml-multicore/eio#635, reviewed by @avsm). This is a minimal initial version. Documentation: - Document `File.Stat` record fields (@avsm @talex5 ocaml-multicore/eio#621). - Update README section about `env` (@talex5 ocaml-multicore/eio#614, reported by @jonsterling). Build and test changes: - Add `File.stat` benchmark (@talex5 ocaml-multicore/eio#616). - Add `Path.stat` benchmark (@patricoferris @talex5 ocaml-multicore/eio#630). - eio_linux: mark as only available on Linux (@talex5 ocaml-multicore/eio#629). - Make MDX tests idempotent (@SGrondin ocaml-multicore/eio#601, reviewed by @talex5). - Allow trailing whitespace in CHANGES.md (@talex5 ocaml-multicore/eio#632). - Update minimum OCaml version to 5.1 (@talex5 ocaml-multicore/eio#631). - Generate prototypes for C stubs from ml files (@talex5 ocaml-multicore/eio#615). - Don't try to compile uring support on centos 7 (@talex5 ocaml-multicore/eio#638, reported by @zenfey).
This is really basic, but it's something that I've found useful to have.
When I first started experimenting with Eio, the only slight pain point I encountered at the very beginning was trying to understand how Flows, Files, Fs, Flow.Buf_read, etc, worked together. Having this simple function would have helped me understand it all.
It can also be useful for debugging.