jruby: Implement set_encoding_by_bom #101
Conversation
This better matches the CRuby logic that calls the equivalent function and brings in BOM support.
Roughly ported from CRuby. See ruby@b249631
headius
force-pushed
the
set_encoding_by_bom
branch
from
23274db
to
939e666
Compare
|
JRuby is still not green but it reduces the failure count by 5, exactly the number of tests that failed due to missing @nobu This is ready for review and merge. I will try to get the remaining tests green some day. |
|
|
||
| // TODO: replace with EncodingUtils#extractModeEncoding |
There was a problem hiding this comment.
Is this still needed?
It seems that we already use EncodingUtils#extractModeEncoding.
There was a problem hiding this comment.
Fixed in upcoming push.
| Object vmodeVperm = EncodingUtils.vmodeVperm(null, null); | ||
| int[] oflags = {0}; | ||
| int[] fmode = {0}; |
There was a problem hiding this comment.
Can we move them to the bellow if (!options.isNil()) { ... } block?
There was a problem hiding this comment.
Fixed in upcoming push, and vmodeVperm and oflags are now allocated once since they are never read by StringIO.
| if (len < 2) break; | ||
| if (toUnsignedInt(bytes[p + 1]) == 0xBB && len > 2) { |
There was a problem hiding this comment.
Just a nitpick: why len < 2?
| if (len < 2) break; | |
| if (toUnsignedInt(bytes[p + 1]) == 0xBB && len > 2) { | |
| if (len < 3) break; | |
| if (toUnsignedInt(bytes[p + 1]) == 0xBB) { |
There was a problem hiding this comment.
The original logic was ported from the C code:
https://github.com/ruby/stringio/blob/master/ext/stringio/stringio.c#L229-L230
Are you sure this should change to len < 3?
There was a problem hiding this comment.
I believe this is resolved in the other comment from @kou.
The code does use extractModeEncoding now.
* Only prepare carrier objects if we'll make the call * Cache the two carrier objects that are actually read (vmodeVperm is read inside extractModeEncoding so can't be a simple constant).
Co-authored-by: Sutou Kouhei <[email protected]>
kou
changed the title
|
Thanks. |
Fix GH-100