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#gets should have same result regardless of #peek availability #13882

Merged
merged 3 commits into from
Oct 12, 2023
Merged

IO#gets should have same result regardless of #peek availability #13882

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
26bf3df
IO#gets_slow should be same regardless of peek availability
compumike Oct 11, 2023
17752d1
IO#gets_slow tracks bytes_read as Bool, not Int32
compumike Oct 11, 2023
c617008
Style: prefer if bytes_read over ternary expression
compumike Oct 11, 2023
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
34 changes: 34 additions & 0 deletions spec/std/io/io_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,40 @@ describe IO do
io.gets(chomp: false).should be_nil
end

it "does gets with empty string (no peek)" do
io = SimpleIOMemory.new("")
io.gets(chomp: true).should be_nil
end

it "does gets with empty string (with peek)" do
io = IO::Memory.new("")
io.gets(chomp: true).should be_nil
end

it "does gets with \\n (no peek)" do
io = SimpleIOMemory.new("\n")
io.gets(chomp: true).should eq("")
io.gets(chomp: true).should be_nil
end

it "does gets with \\n (with peek)" do
io = IO::Memory.new("\n")
io.gets(chomp: true).should eq("")
io.gets(chomp: true).should be_nil
end

it "does gets with \\r\\n (no peek)" do
io = SimpleIOMemory.new("\r\n")
io.gets(chomp: true).should eq("")
io.gets(chomp: true).should be_nil
end

it "does gets with \\r\\n (with peek)" do
io = IO::Memory.new("\r\n")
io.gets(chomp: true).should eq("")
io.gets(chomp: true).should be_nil
end

it "does gets with big line" do
big_line = "a" * 20_000
io = SimpleIOMemory.new("#{big_line}\nworld\n")
Expand Down
11 changes: 8 additions & 3 deletions src/io.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,11 +752,12 @@ abstract class IO

private def gets_slow(delimiter : Char, limit, chomp)
buffer = String::Builder.new
gets_slow(delimiter, limit, chomp, buffer)
buffer.empty? ? nil : buffer.to_s
bytes_read = gets_slow(delimiter, limit, chomp, buffer)
bytes_read.zero? ? nil : buffer.to_s
end

private def gets_slow(delimiter : Char, limit, chomp, buffer : String::Builder) : Nil
private def gets_slow(delimiter : Char, limit, chomp, buffer : String::Builder) : Int32
bytes_read = 0
chomp_rn = delimiter == '\n' && chomp

while true
Expand All @@ -766,6 +767,7 @@ abstract class IO
end

char, char_bytesize = info
bytes_read += char_bytesize

# Consider the case of \r\n when the delimiter is \n and chomp = true
if chomp_rn && char == '\r'
Expand All @@ -776,6 +778,7 @@ abstract class IO
end

char2, char_bytesize2 = info2
bytes_read += char_bytesize2
Copy link
Contributor

@HertzDevil HertzDevil Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think the exact number of bytes read needs to be computed, other than that it is non-zero. So bytes_read can be represented by a Bool instead and this line can be dropped; bytes_read += char_bytesize above already implies at least 1 byte is consumed

Copy link
Member

Choose a reason for hiding this comment

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

Good point.
And actually I think it can be achieved even easier by replacing break in line 766 with return false. That's the only place where the method can return without having read anything. The last line would be true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@HertzDevil I've just pushed a second commit 17752d1 which tracks bytes_read as a Bool and drops the second accumulation bytes_read += char_bytesize2 line as you've suggested.

@straight-shoota If I were to change line 766 from break to return false, I think this might incorrectly return false in any case where we've already read some bytes in previous iterations of the while loop and then hit EOF?

if char2 == '\n'
break
end
Expand All @@ -801,6 +804,8 @@ abstract class IO
break if char_bytesize >= limit
limit -= char_bytesize
end

bytes_read
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing break on line 766 to return false would allow to get rid of the variable altogether.

Suggested change
bytes_read
true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Join same discussion here #13882 (comment) (I'm not sure this would be correct)

end

# Reads until *delimiter* is found or the end of the `IO` is reached.
Expand Down