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

core: assert correct msq queue size on creation #5295

Merged
merged 1 commit into from
Apr 17, 2016

Conversation

OlegHahm
Copy link
Member

The return value was never checked. Hence, this runtime check was rather pointless. Better assert the correct size during development.

@OlegHahm OlegHahm added Area: core Area: RIOT kernel. Handle PRs marked with this with care! Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Apr 11, 2016
@OlegHahm OlegHahm added this to the Release 2016.07 milestone Apr 11, 2016
@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: CI Area: Continuous Integration of RIOT components and removed Area: CI Area: Continuous Integration of RIOT components labels Apr 11, 2016
@kaspar030
Copy link
Contributor

ACK.

@kaspar030
Copy link
Contributor

wow, someone actually did check the return value:

zep:native:
Building application "zep" for "native" with MCU "native".

/data/riotbuild/sys/net/gnrc/application_layer/zep/gnrc_zep.c: In function '_event_loop':
/data/riotbuild/sys/net/gnrc/application_layer/zep/gnrc_zep.c:564:9: error: void value not ignored as it ought to be
     if (msg_init_queue(msg_q, GNRC_ZEP_MSG_QUEUE_SIZE)) {
         ^

@OlegHahm
Copy link
Member Author

Should we grant an award for this? Like "World-1st-msg_init_queue-Checker"?

Fixed.

cib_init(&(me->msg_queue), num);
return 0;
}
assert(num && ((num & (num - 1)) == 0));
Copy link
Member

Choose a reason for hiding this comment

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

why? cib_init is already asserting that. And the only reason for asserting that here is, because the msg_queue uses cib?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, didn't check this. Well, then I think we can remove the check completely. 👍

@OlegHahm
Copy link
Member Author

addressed

@kaspar030
Copy link
Contributor

pls squash!

@cgundogan
Copy link
Member

regarding the assert: the cib only checks for power of 2 values, but also allows 0 in the assert. The former assert in msg prevented 0. Maybe use an assert(num) instead in msg_init_queue?

@OlegHahm
Copy link
Member Author

regarding the assert: the cib only checks for power of 2 values, but also allows 0 in the assert. The former assert in msg prevented 0. Maybe use an assert(num) instead in msg_init_queue?

I noticed that, but I came to the conclusion that calling msg_init_queue() with size zero sounds really esoteric.

@miri64
Copy link
Member

miri64 commented Apr 17, 2016

ACK by @kaspar030 seconded.

The return value was never checked. Hence, this runtime check was rather
pointless. Better assert the correct size during development.
@OlegHahm
Copy link
Member Author

squashed

@OlegHahm OlegHahm merged commit 8690a88 into RIOT-OS:master Apr 17, 2016
@OlegHahm OlegHahm deleted the msg_init_queue_assert branch April 17, 2016 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants