Skip to content
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

refactor example/c/lib_buffer_unpack.c #344

Merged
merged 4 commits into from
Aug 26, 2015
Merged

Conversation

n1tehawk
Copy link
Contributor

The example has some duplicated code that somewhat distracts from
the main processing loop. I think placing this into a separate
function improves readability of the code.

The example has some duplicated code that somewhat distracts from
the main processing loop. I think placing this into a separate
function improves readability of the code.
@n1tehawk
Copy link
Contributor Author

CI tests indicate that the previous commit made the "buf" variable obsolete (unused), potentially causing warnings.
n1tehawk@871a796 fixes that

{
// make sure there's enough room, or expand the unpacker accordingly
if (msgpack_unpacker_buffer_capacity(unpacker) < request_size) {
assert(msgpack_unpacker_reserve_buffer(unpacker, request_size));
Copy link
Member

Choose a reason for hiding this comment

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

assert macro can be disabled and this line could completely be removed. So, this has to be like the original code:

bool expanded = msgpack_unpacker_reserve_buffer(unpacker, request_size);
assert(expanded);

Copy link
Member

Choose a reason for hiding this comment

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

and it generates unused variable error (-Werror) :) umm. I'll talk to @redboltz about this.

Copy link
Member

Choose a reason for hiding this comment

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

msgpack_unpacker_reserve_buffer(unpacker, request_size)
    || (assert(0 && "msgpack_unpacker_reserve_buffer failed"), 0);

@redboltz Is this acceptable to you? Do you have a better idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh... But that's a problem that also affects the original code, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since msgpack-c already has a MSGPACK_UNUSED macro in https://github.com/msgpack/msgpack-c/blob/master/include/msgpack/util.h#L21, it might be a saner approch to simply mark the local variable as "potentially unused":

bool expanded = msgpack_unpacker_reserve_buffer(unpacker, request_size);
assert(expanded);
MSGPACK_UNUSED(expanded);

EDIT:
A "clean" way to solve this is command-query separation. In this particular case that's easy to achieve:

if (msgpack_unpacker_buffer_capacity(unpacker) < request_size) {
    msgpack_unpacker_reserve_buffer(unpacker, request_size);
    assert(msgpack_unpacker_buffer_capacity(unpacker) >= request_size);
}

@nobu-k
Copy link
Member

nobu-k commented Aug 17, 2015

Your code looks much nicer than the previous one. Thanks! I'll merge this after fixing the problem above.

@n1tehawk
Copy link
Contributor Author

Thanks! :) And nice catch with the assert() evaluation.

We might even further reduce the reading loop to a single call of receiver_to_unpacker(), like this;

while (true) {
    recv_len = receiver_to_unpacker(r, EACH_RECV_SIZE, unp);
    if (recv_len == 0) break; // (reached end of input)

Regards, NiteHawk

@nobu-k
Copy link
Member

nobu-k commented Aug 17, 2015

We might even further reduce the reading loop to a single call of receiver_to_unpacker(), like this;

I think it seems better. I'll apply the change when I merge this PR.

There's a small problem remaining if assertions are disabled (with -DNDEBUG).
@n1tehawk
Copy link
Contributor Author

I've incorporated the suggested changes into another commit. The only remaining issue that needs to be addressed is that assert() logic.

@nobu-k
Copy link
Member

nobu-k commented Aug 17, 2015

Thank you!

@n1tehawk
Copy link
Contributor Author

One more thing: I'm wondering if unpack() (in lib_buffer_unpack.c) is missing a msgpack_unpacker_free(unp); at the end?

@redboltz
Copy link
Contributor

One more thing: I'm wondering if unpack() (in lib_buffer_unpack.c) is missing a msgpack_unpacker_free(unp); at the end?

@n1tehawk, thank you for pointing out the problem. You are right. I checked the executable using valgrind, then resource leaks are detected.

@n1tehawk
Copy link
Contributor Author

Hi @redboltz!

I'll prepare another commit then - which will contain the free() call, and also addresses the assert() stuff discussed before.

redboltz added a commit that referenced this pull request Aug 26, 2015
refactor example/c/lib_buffer_unpack.c
@redboltz redboltz merged commit 95e0fc5 into msgpack:master Aug 26, 2015
@redboltz
Copy link
Contributor

Merged. Thanks you @n1tehawk :)

@n1tehawk n1tehawk deleted the contrib branch August 28, 2015 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants