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

use rcutils allocator in allocators.c to avoid direct use of malloc/free #140

Merged
merged 1 commit into from
Jun 17, 2018

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Jun 15, 2018

This pull request makes sure that we're using rcutils_allocator_t everywhere and not using malloc, realloc, calloc, or free anywhere at all.

Connects to ros2/rcutils#102

@wjwwood wjwwood added enhancement New feature or request in review Waiting for review (Kanban column) labels Jun 15, 2018
@wjwwood wjwwood self-assigned this Jun 15, 2018
@wjwwood
Copy link
Member Author

wjwwood commented Jun 15, 2018

I was a little worried about a performance impact with this change (as the rcutils allocator is across a shared library boundary most of the time), but I was looking at the decompiled code and apparently it jumps directly to __default_allocate in allocators.c of rcutils from rmw_allocate(). I think the compiler is smart enough to know that what rcutils_get_default_allocator() returns is stored locally as static and is unmodified, so it makes it const, and because rmw_allocate also doesn't modify the copy of it, the compiler just removes it all together and goes where .allocate points with what .state points to.

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

LGTM

@wjwwood wjwwood merged commit ea9c28e into master Jun 17, 2018
@wjwwood wjwwood deleted the dynamic_memory_audit branch June 17, 2018 05:06
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Jun 17, 2018
dabonnie pushed a commit to aws-ros-dev/rmw that referenced this pull request Apr 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants