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

Implement Zlib::GzipReader #ungetbyte and #ungetc #4636

Merged
merged 4 commits into from Jun 2, 2017

Conversation

Projects
None yet
3 participants
@haines
Contributor

haines commented May 31, 2017

I've had a stab at implementing the unget* methods for Zlib::GzipReader. The implementation is a bit limited in that the maximum number of bytes that can be "ungot" is hardcoded, since I'm using a PushbackInputStream. Also, it doesn't attempt to do anything clever with encoding (although I don't see that as a problem because getc doesn't either).

Let me know what you think!

Closes #4631

@haines

This comment has been minimized.

Show comment
Hide comment
@haines

haines May 31, 2017

Contributor

I think the MRI zlib test failure (here) is actually an issue with the implementation of gets("") -- MRI skips the leading newline but JRuby does not.

Contributor

haines commented May 31, 2017

I think the MRI zlib test failure (here) is actually an issue with the implementation of gets("") -- MRI skips the leading newline but JRuby does not.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 31, 2017

Member

@haines You may be right. Do you want to look into it? It may just be some missing logic in our ported GzipReader#gets implementation. I suppose the test passed before because it fell back on io#ungetc which didn't actually unget anything, so the subsequent gets worked properly.

Thanks for your help! I have some review comments I'll add.

Member

headius commented May 31, 2017

@haines You may be right. Do you want to look into it? It may just be some missing logic in our ported GzipReader#gets implementation. I suppose the test passed before because it fell back on io#ungetc which didn't actually unget anything, so the subsequent gets worked properly.

Thanks for your help! I have some review comments I'll add.

Show outdated Hide outdated core/src/main/java/org/jruby/ext/zlib/JZlibRubyGzipReader.java
import org.jruby.RubyIO;
import org.jruby.RubyNumeric;
import org.jruby.RubyString;
import org.jruby.*;

This comment has been minimized.

@headius

headius May 31, 2017

Member

We generally don't collapse imports unless there's >20 or something.

@headius

headius May 31, 2017

Member

We generally don't collapse imports unless there's >20 or something.

Show outdated Hide outdated test/jruby/test_zlib.rb
@@ -517,6 +517,24 @@ def test_error_input
assert_equal("not in gzip format", e.message)
assert_equal("foobarzothoge", e.input)
end
def test_gzip_reader_ungetc

This comment has been minimized.

@headius

headius May 31, 2017

Member

If these don't exist in either MRI's suite or ruby/spec, it would be nice to add them there. We usually just put JRuby-specific stuff in test/jruby.

@headius

headius May 31, 2017

Member

If these don't exist in either MRI's suite or ruby/spec, it would be nice to add them there. We usually just put JRuby-specific stuff in test/jruby.

This comment has been minimized.

@haines

haines May 31, 2017

Contributor

Makes sense. There are placeholders for these methods in ruby/spec, so I'll add them there.

Should I submit them to ruby/spec first and then pull them in once merged upstream?

@haines

haines May 31, 2017

Contributor

Makes sense. There are placeholders for these methods in ruby/spec, so I'll add them there.

Should I submit them to ruby/spec first and then pull them in once merged upstream?

This comment has been minimized.

@headius

headius May 31, 2017

Member

You can submit them to our spec/ruby subdir. We merge both ways.

@headius

headius May 31, 2017

Member

You can submit them to our spec/ruby subdir. We merge both ways.

This comment has been minimized.

@headius

headius May 31, 2017

Member

Make it a separate commit for sure though...makes it easier to merge. 😄

@headius

headius May 31, 2017

Member

Make it a separate commit for sure though...makes it easier to merge. 😄

@headius headius added this to the JRuby 9.1.11.0 milestone May 31, 2017

@haines

This comment has been minimized.

Show comment
Hide comment
@haines

haines May 31, 2017

Contributor

I've added the specs, which caught a bug in my implementation (I forgot to decrement pos), and a couple in MRI:

  • ungetting at the start of the stream causes pos to underflow (I've raised this: https://bugs.ruby-lang.org/issues/13616)
  • ungetting nil raises a TypeError: no implicit conversion of nil. I'm not sure if I should raise this as a bug in MRI or leave the behaviour unspecified. File accepts nil and silently ignores it, so I think GzipReader should behave in the same way for consistency. On the other hand, trying to unget nil is a strange thing to do, so raising a TypeError is fairly reasonable. What do you think?

I'll take a look at the #gets implementation tomorrow (UK time).

Contributor

haines commented May 31, 2017

I've added the specs, which caught a bug in my implementation (I forgot to decrement pos), and a couple in MRI:

  • ungetting at the start of the stream causes pos to underflow (I've raised this: https://bugs.ruby-lang.org/issues/13616)
  • ungetting nil raises a TypeError: no implicit conversion of nil. I'm not sure if I should raise this as a bug in MRI or leave the behaviour unspecified. File accepts nil and silently ignores it, so I think GzipReader should behave in the same way for consistency. On the other hand, trying to unget nil is a strange thing to do, so raising a TypeError is fairly reasonable. What do you think?

I'll take a look at the #gets implementation tomorrow (UK time).

haines added some commits May 30, 2017

@haines

This comment has been minimized.

Show comment
Hide comment
@haines

haines Jun 1, 2017

Contributor

I think this is good to go (assuming you're happy with it!). Turns out #gets("") is supposed to skip any number of leading or trailing newlines, so I've added a spec for that behaviour too.

Contributor

haines commented Jun 1, 2017

I think this is good to go (assuming you're happy with it!). Turns out #gets("") is supposed to skip any number of leading or trailing newlines, so I've added a spec for that behaviour too.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jun 2, 2017

Member

Looks good now...I'll merge!

Member

headius commented Jun 2, 2017

Looks good now...I'll merge!

@headius headius merged commit 15899bb into jruby:master Jun 2, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@haines haines deleted the haines:gzip_reader_unget branch Jun 2, 2017

haines added a commit to haines/tar that referenced this pull request Jun 6, 2017

@eregon

This comment has been minimized.

Show comment
Hide comment
@eregon

eregon Jun 23, 2017

Member

@headius @haines

ungetting nil raises a TypeError: no implicit conversion of nil. I'm not sure if I should raise this as a bug in MRI or leave the behaviour unspecified. File accepts nil and silently ignores it, so I think GzipReader should behave in the same way for consistency. On the other hand, trying to unget nil is a strange thing to do, so raising a TypeError is fairly reasonable. What do you think?

Either follow what MRI does, or raise a bug, but please not spec different behavior without a bug report.
I cannot keep these specs in ruby/spec since they don't pass on MRI and there is no matching MRI bug.
I'll write a bug report and put these specs in quarantine in the meantime.

Member

eregon commented Jun 23, 2017

@headius @haines

ungetting nil raises a TypeError: no implicit conversion of nil. I'm not sure if I should raise this as a bug in MRI or leave the behaviour unspecified. File accepts nil and silently ignores it, so I think GzipReader should behave in the same way for consistency. On the other hand, trying to unget nil is a strange thing to do, so raising a TypeError is fairly reasonable. What do you think?

Either follow what MRI does, or raise a bug, but please not spec different behavior without a bug report.
I cannot keep these specs in ruby/spec since they don't pass on MRI and there is no matching MRI bug.
I'll write a bug report and put these specs in quarantine in the meantime.

@eregon

This comment has been minimized.

Show comment
Hide comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment