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

Always read unbuffered when IO::Buffered#sync = true #5849

Merged
merged 3 commits into from
May 22, 2018
Merged

Always read unbuffered when IO::Buffered#sync = true #5849

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4b66657
Always read unbuffered when IO::Buffered#sync = true
RX14 Mar 21, 2018
1283bfb
fixup! Always read unbuffered when IO::Buffered#sync = true
RX14 Mar 23, 2018
2817969
fixup! Always read unbuffered when IO::Buffered#sync = true
RX14 May 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 91 additions & 9 deletions spec/std/io/buffered_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,19 +284,101 @@ describe "IO::Buffered" do
str.to_s.should eq("abcd")
end

it "syncs" do
str = IO::Memory.new
describe "sync" do
it "syncs (write)" do
str = IO::Memory.new

io = BufferedWrapper.new(str)
io.sync?.should be_false
io = BufferedWrapper.new(str)
io.sync?.should be_false

io.sync = true
io.sync?.should be_true

io.write_byte 1_u8

str.rewind
str.read_byte.should eq(1_u8)
end

it "works with IO#read" do
str = IO::Memory.new "abc"

io = BufferedWrapper.new(str)
io.sync?.should be_false

io.sync = true
io.sync?.should be_true

byte = Bytes.new(1)
io.read_fully(byte)
byte[0].should eq('a'.ord.to_u8)

str.gets_to_end.should eq("bc")
end

it "works with IO#read (already buffered)" do
str = IO::Memory.new
str << "a" * IO::Buffered::BUFFER_SIZE
str.pos = 0

io = BufferedWrapper.new(str)
io.sync?.should be_false

IO::Buffered::BUFFER_SIZE.times do
byte = Bytes.new(1)
io.read_fully(byte)
byte[0].should eq('a'.ord.to_u8)
end

io.sync = true
io.sync?.should be_true

io.sync = true
io.sync?.should be_true
str << "bcde"
str.pos -= 4

io.write_byte 1_u8
byte = Bytes.new(1)
io.read_fully(byte)
byte[0].should eq('b'.ord.to_u8)

str.rewind
str.read_byte.should eq(1_u8)
str.gets_to_end.should eq("cde")
end

it "works with IO#read_byte" do
str = IO::Memory.new "abc"

io = BufferedWrapper.new(str)
io.sync?.should be_false

io.sync = true
io.sync?.should be_true

io.read_byte.should eq('a'.ord.to_u8)

str.gets_to_end.should eq("bc")
end

it "works with IO#read_byte (already buffered)" do
str = IO::Memory.new
str << "a" * IO::Buffered::BUFFER_SIZE
str.pos = 0

io = BufferedWrapper.new(str)
io.sync?.should be_false

IO::Buffered::BUFFER_SIZE.times do
io.read_byte.should eq('a'.ord.to_u8)
end

io.sync = true
io.sync?.should be_true

str << "bcde"
str.pos -= 4

io.read_byte.should eq('b'.ord.to_u8)

str.gets_to_end.should eq("cde")
end
end

it "shouldn't call unbuffered read if reading to an empty slice" do
Expand Down
13 changes: 10 additions & 3 deletions src/io/buffered.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,16 @@ module IO::Buffered
def read_byte : UInt8?
check_open

fill_buffer if @in_buffer_rem.empty?
fill_buffer if !sync? && @in_buffer_rem.empty?
if @in_buffer_rem.empty?
nil
return nil unless sync?

byte = uninitialized UInt8
if read(Slice.new(pointerof(byte), 1)) == 1
Copy link
Contributor

@Sija Sija Mar 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

byte if read(Slice.new(pointerof(byte), 1)) == 1? Than you could skip else branch altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer my more explicit version.

Copy link
Contributor

Choose a reason for hiding this comment

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

No. An else nil branch is always useless. Please drop it.

Copy link
Contributor

@ysbaddaden ysbaddaden Mar 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Same for the return nil above. return always returns nil. Skip the noise.

Copy link
Contributor Author

@RX14 RX14 Mar 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I thought we agreed to skip the pointless bikeshedding about style. This kind of shit just drives away contributors. Until someone puts in the effort for an official style guide, the formatter is the style guide. And this formats.

Copy link
Contributor Author

@RX14 RX14 Mar 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Also note that I copied this directly from io.cr:

crystal/src/io.cr

Lines 302 to 309 in 2d93603

def read_byte : UInt8?
byte = uninitialized UInt8
if read(Slice.new(pointerof(byte), 1)) == 1
byte
else
nil
end
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Formatter ain't a rewritter.

I never accepted the introduction of useless nils, and will always ask for them to be removed. Copying noise isn't a reason to keep it.

byte
else
nil
end
else
b = @in_buffer_rem[0]
@in_buffer_rem += 1
Expand All @@ -54,7 +61,7 @@ module IO::Buffered
# If we are asked to read more than half the buffer's size,
# read directly into the slice, as it's not worth the extra
# memory copy.
if count >= BUFFER_SIZE / 2
if sync? || count >= BUFFER_SIZE / 2
return unbuffered_read(slice[0, count]).to_i
else
fill_buffer
Expand Down