-
Notifications
You must be signed in to change notification settings - Fork 568
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
Add ProtoReader
API for length-delimited stream reads
#2747
Conversation
check(state == STATE_TAG || state == STATE_LENGTH_DELIMITED) { | ||
"Unexpected call to nextDelimited()" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear this is useful. Could eliminate and only have the one function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make it clearer by telling which state we're in? I don't mind whether we keep it or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message is the same as other places in this file. I can update them all separately to include the current state.
b06b8d0
to
cf6698e
Compare
@@ -32,4 +32,25 @@ class ProtoReaderTest { | |||
assertEquals(-1, reader.nextTag()) | |||
reader.endMessageAndGetUnknownFields(token) | |||
} | |||
|
|||
@Test fun lengthDelimited() { | |||
val encoded = "0208020608ffffffff07".decodeHex() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
val encoded = "0208020608ffffffff07".decodeHex() | |
// Bytes representing: | |
// ── 0 ┐ | |
// ╰- 0 ┐ | |
// ╰- 1: 2147483647 | |
val encoded = "0208020608ffffffff07".decodeHex() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a single proto. It's two protos prefixed with their varint32-encoded lengths. I'll add a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sweet
check(state == STATE_TAG || state == STATE_LENGTH_DELIMITED) { | ||
"Unexpected call to nextDelimited()" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make it clearer by telling which state we're in? I don't mind whether we keep it or not
cf6698e
to
2ea68d1
Compare
Closes #633