Skip to content

add zlib each_char #8608

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

Merged
headius merged 1 commit into from
Feb 8, 2025
Merged

add zlib each_char #8608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 21 additions & 0 deletions core/src/main/java/org/jruby/ext/zlib/JZlibRubyGzipReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,6 +752,27 @@ public IRubyObject each_byte(ThreadContext context, Block block) {
return context.nil;
}

@JRubyMethod
public IRubyObject each_char(ThreadContext context, Block block) {
final Ruby runtime = context.runtime;
if (!block.isGiven()) return RubyEnumerator.enumeratorize(runtime, this, "each_char");

try {
int value = bufferedStream.read();
while(value != -1) {
position++;
// TODO: must handle encoding. Move encoding handling methods to util class from RubyIO and use it.
// TODO: StringIO needs a love, too.
block.yield(context, runtime.newString(String.valueOf((char) (value & 0xFF))));

@enebo enebo Jan 31, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I totally understand how this code got here looking at getc but this has to be wrong as it uses a byte-oriented API and returns it as a char. Perhaps no one does multiple byte chars using this API as I would think #getc would have had a report by now since this has that same logic in it.

Another amusing thing to notice is MRI unit tests have no tests for this method. No wonder we are missing it. You will be the first person to actually add a test for this!

I am ok landing this if you open an issue that both each_char and getc is not recreating chars but is using bytes. It appears initialize will process opts and set encoding so I think we just need to use that and do something like RubyString#enumerateChars where we use StringSupport to look for length and that many bytes as a new char.

@enebo enebo Jan 31, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I should add since there are no tests perhaps each_char really does only return bytes. I would be a pretty big misnaming if so.

@danini-the-panini danini-the-panini Feb 11, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In MRI it does return multi-byte chars, and should work the same as calling each_char on the inflated string

@enebo enebo Feb 11, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@danini-the-panini so then perhaps you want to take a look at fixing that for both each_char and getc in zlib? It will largely just be looking at our String-related code and doing the same thing.

@enebo enebo Feb 11, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(as a new issue)

@danini-the-panini danini-the-panini Feb 11, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Issue created: #8621

value = bufferedStream.read();
}
} catch(IOException ioe) {
throw runtime.newIOErrorFromException(ioe);
}

return context.nil;
}

/**
* Document-method: Zlib::GzipReader.zcat
*
Expand Down
51 changes: 51 additions & 0 deletions spec/ruby/library/zlib/gzipreader/each_char_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
require_relative '../../../spec_helper'
require 'stringio'
require 'zlib'

describe "Zlib::GzipReader#each_char" do

before :each do
@data = '12345abcde'
@zip = [31, 139, 8, 0, 44, 220, 209, 71, 0, 3, 51, 52, 50, 54, 49, 77,
76, 74, 78, 73, 5, 0, 157, 5, 0, 36, 10, 0, 0, 0].pack('C*')

@io = StringIO.new @zip
ScratchPad.clear
end

it "calls the given block for each char in the stream, passing the char as an argument" do
gz = Zlib::GzipReader.new @io

ScratchPad.record []
gz.each_char { |b| ScratchPad << b }

ScratchPad.recorded.should == ["1", "2", "3", "4", "5", "a", "b", "c", "d", "e"]
end

it "returns an enumerator, which yields each char in the stream, when no block is passed" do
gz = Zlib::GzipReader.new @io
enum = gz.each_char

ScratchPad.record []
while true
begin
ScratchPad << enum.next
rescue StopIteration
break
end
end

ScratchPad.recorded.should == ["1", "2", "3", "4", "5", "a", "b", "c", "d", "e"]
end

it "increments position before calling the block" do
gz = Zlib::GzipReader.new @io

i = 1
gz.each_char do |ignore|
gz.pos.should == i
i += 1
end
end

end