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

Implement get_rcl_allocator. #467

Open
wants to merge 2 commits into
base: rolling
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions demo_nodes_cpp/src/topics/allocator_tutorial.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,33 @@ constexpr bool operator!=(
return false;
}

// You need to overload get_rcl_allocator for custom allocators.
// This function needs to build an instance of rcl_allocator_t
// for use in C code.
template<typename T>
rcl_allocator_t get_rcl_allocator(MyAllocator<T>)
Copy link
Member

@wjwwood wjwwood Oct 26, 2020

Choose a reason for hiding this comment

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

Doesn't this need to be in the rclcpp::allocator namespace?

Copy link
Author

Choose a reason for hiding this comment

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

Argument-dependent lookup saves us: https://en.cppreference.com/w/cpp/language/adl. As long as the function is in the namespace of one of its arguments, the overload will be resolved.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but should it be in the same namespace... I feel like the answer is yes, but I'm open to suggestion. I'm thinking from the perspective of a person reading this code and not knowing what it is exactly that is being overridden or where it comes from. The namespace gives important context imo.

Copy link
Author

Choose a reason for hiding this comment

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

Got you. I don't care one way or another, so if you stumble over the namespace, it's probably a good signal that many devs would stumble over the namespace. Moved it to rclcpp.

rclcpp::allocator wouldn't work because the calling code is in rclcpp.

Copy link
Member

Choose a reason for hiding this comment

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

It should be in the rclcpp::allocator namespace, I commented on the original pr: ros2/rclcpp#1324 (review)

{
rcl_allocator_t rcl_allocator;
rcl_allocator.allocate = [](size_t size, void *) {
num_allocs++;
return malloc(size);
};
rcl_allocator.deallocate = [](void * pointer, void *) {
num_deallocs++;
return free(pointer);
};
rcl_allocator.reallocate = [](void * pointer, size_t size, void *) {
num_allocs++;
num_deallocs++;
return realloc(pointer, size);
};
rcl_allocator.zero_allocate = [](size_t nmemb, size_t size, void *) {
num_allocs++;
return calloc(nmemb, size);
};
return rcl_allocator;
}

// Override global new and delete to count calls during execution.

static bool is_running = false;
Expand Down