Conversation
danini-the-panini
force-pushed
the
zlib-fixes
branch
from
f5d0f17 to
41459fe
Compare
enebo
left a comment
enebo
left a comment
There was a problem hiding this comment.
shoot I forgot to submit this and as I said in comments this is landable as-is. Just adding these comments for posterity.
| 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)))); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
In MRI it does return multi-byte chars, and should work the same as calling each_char on the inflated string
There was a problem hiding this comment.
@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.
3 participants
Resolves #8351