Skip to content

Fix packString #359

Merged
xerial merged 6 commits intodevelopfrom
test-str8-disable
Apr 11, 2016
Merged

Fix packString #359
xerial merged 6 commits intodevelopfrom
test-str8-disable

Conversation

@xerial
Copy link
Copy Markdown
Member

@xerial xerial commented Apr 8, 2016

While testing the config for disabling STR8 output, I found an unreachable code path in packString. This PR fixes the usage of CharsetEncoder used in packString.

@xerial xerial changed the title Fix unpackString Fix packString Apr 8, 2016
@xerial xerial force-pushed the test-str8-disable branch from 3f7c337 to dfcf588 Compare April 8, 2016 17:48
@xerial
Copy link
Copy Markdown
Member Author

xerial commented Apr 8, 2016

@frsyuki Could you check this PR? I found CharsetEncoder.underflow is reported even if the entire input string is properly consumed. For example, here is a code of StringCoding.encode:

                ce.reset();
                ByteBuffer bb = ByteBuffer.wrap(ba);
                CharBuffer cb = CharBuffer.wrap(ca, off, len);
                try {
                    CoderResult cr = ce.encode(cb, bb, true);
                    if (!cr.isUnderflow())
                        cr.throwException();
                    cr = ce.flush(bb);
                    if (!cr.isUnderflow())
                        cr.throwException();
                } catch (CharacterCodingException x) {
                    // Substitution is always enabled,
                    // so this shouldn't happen
                    throw new Error(x);
                }

which is used in String.getBytes of Java.

@xerial xerial mentioned this pull request Apr 8, 2016
Merged
frsyuki
frsyuki reviewed Apr 11, 2016
View reviewed changes
Comment thread msgpack-core/src/main/java/org/msgpack/core/MessagePacker.java Outdated
// Underflow should be on to ensure all of the input string is encoded
return -1;
}
// NOTE: This flush method does nothing if we use UTR8 encoder, but other general encoders require this
Copy link
Copy Markdown
Member

@frsyuki frsyuki Apr 11, 2016

Choose a reason for hiding this comment

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

what's UTR8 encoder?

Copy link
Copy Markdown
Member

@frsyuki frsyuki Apr 11, 2016

Choose a reason for hiding this comment

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

oh, I think you mean UT"F"8

Copy link
Copy Markdown
Member Author

@xerial xerial Apr 11, 2016

Choose a reason for hiding this comment

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

ah. I'll fix this.

@frsyuki
Copy link
Copy Markdown
Member

frsyuki commented Apr 11, 2016

ok, I see......that's very good catch.
I confirmed it by using jruby.
I commented some. others look good!

@xerial
Copy link
Copy Markdown
Member Author

xerial commented Apr 11, 2016

Applied the comment. Thanks!

@xerial xerial merged commit 202ca62 into develop Apr 11, 2016
@xerial xerial deleted the test-str8-disable branch July 19, 2025 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@xerial @frsyuki