Conversation
This is needed as a companion change to rclcpp #1324. Signed-off-by: Steve Wolter <swolter@google.com>
| // 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>) |
There was a problem hiding this comment.
Doesn't this need to be in the rclcpp::allocator namespace?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It should be in the rclcpp::allocator namespace, I commented on the original pr: ros2/rclcpp#1324 (review)
Signed-off-by: Steve Wolter <swolter@google.com>
2 participants
This is needed as a companion change to ros2/rclcpp#1324.
Signed-off-by: Steve Wolter swolter@google.com