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

io: add a BufferedWriter and supporting methods #22265

Merged
merged 12 commits into from
Sep 22, 2024
Merged

io: add a BufferedWriter and supporting methods #22265

merged 12 commits into from
Sep 22, 2024

Conversation

einar-hjortdal
Copy link
Contributor

Address #21975

@einar-hjortdal
Copy link
Contributor Author

oops. I messed up the rebase.

@einar-hjortdal einar-hjortdal force-pushed the feat_buffered-writer branch 3 times, most recently from 8f04db7 to 8cb24cc Compare September 21, 2024 17:23
einar-hjortdal and others added 5 commits September 22, 2024 09:27
@einar-hjortdal
Add buffered writer
64b2929 
Signed-off-by: Einar Hjortdal <einar.hjortdal@outlook.com>
@einar-hjortdal @spytheman
Apply suggestions from code review
7f938fc 
drop `arrays.concat` in favor of `<<`

Co-authored-by: Delyan Angelov <delian66@gmail.com>
Signed-off-by: Einar Hjortdal <einar.hjortdal@outlook.com>
@einar-hjortdal
Imply array cap
fd0dc59 
Signed-off-by: Einar Hjortdal <einar.hjortdal@outlook.com>
@einar-hjortdal
Add documentation to buffered_writer
9fcabc2 
Signed-off-by: Einar Hjortdal <einar.hjortdal@outlook.com>
@einar-hjortdal
Fix infinite loop situation
029dc8b
@einar-hjortdal
Copy link
Contributor Author

Test is failing sometimes, I think I wrote a bad utility function in the test and just discovered it.

@spytheman
Copy link
Member

The windows CI failures are unrelated and are already fixed on master.

spytheman
spytheman approved these changes Sep 22, 2024
View reviewed changes
Copy link
Member

@spytheman spytheman left a comment

Choose a reason for hiding this comment

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

Excellent work.

I could not think about any other tests.

spytheman
spytheman reviewed Sep 22, 2024
View reviewed changes
vlib/io/buffered_writer_test.v
import io
import rand

struct ArrayWriter {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
struct ArrayWriter {
struct ArrayWriter implements Writer {

Copy link
Member

Choose a reason for hiding this comment

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

although that could wait a bit more 🤔

spytheman
spytheman reviewed Sep 22, 2024
View reviewed changes
vlib/io/buffered_writer.v
Comment on lines +51 to +54
n := b.wr.write(b.buf[0..b.n])!
if n < b.n {
return error('Writer accepted less bytes than expected without returning any explicit error.')
}
Copy link
Member

Choose a reason for hiding this comment

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

we could perhaps loop here, till everything is flushed

Copy link
Member

Choose a reason for hiding this comment

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

or there could be a separate flush_all method that does it 🤔

@spytheman spytheman changed the title io: add buffered writer io: add a BufferedWriter and supporting methods Sep 22, 2024
@spytheman spytheman merged commit 64d1770 into vlang:master Sep 22, 2024
64 of 67 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spytheman spytheman spytheman approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@einar-hjortdal @spytheman