thrift: transport and protocol write code#3789
Conversation
Matches the read side implementations. Transport writes require having the entire protocol message and so the write methods do not mirror reads. *Risk Level*: low, code not used *Testing*: unit tests *Docs Changes*: n/a *Release Notes*: n/a Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
brian-pane
left a comment
There was a problem hiding this comment.
Thanks for this PR. This looks good overall. I'm still reading through it, but here are my (minor) comments and questions on the first half.
| return; | ||
| } | ||
|
|
||
| BufferHelper::writeI16(buffer, field_id); |
There was a problem hiding this comment.
Negative values for field_id are okay, right?
There was a problem hiding this comment.
According to the protocol specs, it's a signed, 16-bit integer. The thrift code generator will not let you choose negative values or 0. That said, I think I'll allow negative values, just in case.
|
|
||
| BufferHelper::writeI8(buffer, static_cast<int8_t>(key_type)); | ||
| BufferHelper::writeI8(buffer, static_cast<int8_t>(value_type)); | ||
| BufferHelper::writeI32(buffer, static_cast<int32_t>(size)); |
There was a problem hiding this comment.
Should we verify that size is nonnegative? (Same question applies to writeListBegin and writeSetBegin)
There was a problem hiding this comment.
There's a comparison of size against INT32_MAX (which is 0x7fffffff) which guarantees that static_cast<int32_t>(size) is non-negative.
brian-pane
left a comment
There was a problem hiding this comment.
I just read through the rest, and it looks ok to me.
mattklein123
left a comment
There was a problem hiding this comment.
LGTM from skim. Small optional nit. Can do in followup if you want.
|
|
||
| void BinaryProtocolImpl::writeMapBegin(Buffer::Instance& buffer, FieldType key_type, | ||
| FieldType value_type, uint32_t size) { | ||
| if (size > INT32_MAX) { |
There was a problem hiding this comment.
nit: std::numeric_limits here and elsewhere?
There was a problem hiding this comment.
I'll do this in a follow-up.
Matches the read side implementations. Transport writes require having the
entire protocol message and so the write methods do not mirror reads. This
is further preparation for a Thrift router that can translate requests between
transports and protocols.
Risk Level: low, code not used
Testing: unit tests
Docs Changes: n/a
Release Notes: n/a
Signed-off-by: Stephan Zuercher stephan@turbinelabs.io