-
Notifications
You must be signed in to change notification settings - Fork 16
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
Frame: decode payload length to return early #31
Conversation
Signed-off-by: Evgeny Malygin <[email protected]>
looking at this again, I'm not 100% sure we don't rely on the type, channel being populated on a partially returned frame - @adamncasey do you have any thoughts here. |
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.
I also wasn't sure on this change but your question made me look in a few places which convinced me:
https://github.com/bloomberg/rmqcpp/blob/main/src/rmq/rmqio/rmqio_decoder.cpp#L55
If we return PARTIAL we never look at the (possibly) populated frame object.
https://github.com/bloomberg/rmqcpp/blob/main/src/tests/rmqio/rmqio_decoder.t.cpp#L71 this test covers this behaviour
I believe I reviewed Another thing, we have a partial rmqcpp/src/rmq/rmqamqpt/rmqamqpt_frame.cpp Lines 59 to 67 in 98a7e38
when no fields are updated. This means, that the old version of the code has a strange situation: PARTIAL means that some fields might be or might be not initialized. And we still don't have a way to know which fields are initialized in this case. That's the reason why not initializing fields for partial Frame in all situation makes it more clear. |
A small optimization for partial
Frame
s.I have a feeling though that it makes things a bit more complicated, especially addressing buffer via exact offsets like
buffer[3]
.Might be good to add
const size_t
constants for these offsets.Replacement for PR with Verified status