Skip to content

Conversation

@jacquelinekay
Copy link
Contributor

While exposing the allocator template in rclcpp, I noticed our macro definitions have trouble when there are commas in a type (i.e. when there are multiple template arguments). I decided to change all the macro definitions to variadic macros, which will allow commas in the type names.

@jacquelinekay jacquelinekay added the in progress Actively being worked on (Kanban column) label Oct 9, 2015
@jacquelinekay jacquelinekay self-assigned this Oct 9, 2015
@jacquelinekay jacquelinekay added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Oct 9, 2015
@esteve
Copy link
Member

esteve commented Oct 9, 2015

+1

jacquelinekay added a commit that referenced this pull request Oct 10, 2015
@jacquelinekay jacquelinekay merged commit 8e4cc7c into master Oct 10, 2015
@jacquelinekay jacquelinekay removed the in review Waiting for review (Kanban column) label Oct 10, 2015
@jacquelinekay jacquelinekay deleted the use_variadic_macros branch October 10, 2015 00:44
Copy link
Member

Choose a reason for hiding this comment

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

This looks weird to me. The template accepts multiple arguments now (e.g. Foo, Bar) which then ends up as const Foo, Bar &?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. I wasn't sure how else to allow passing a class with multiple template arguments to these macros.

nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022
…ondition_tests

reenable tests for rcl_node_get_graph_guard_condition for all rmw impl
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022
mauropasse pushed a commit to mauropasse/rclcpp that referenced this pull request Oct 3, 2023
…iron

Do not crash Executor when send_response fails due to client failure.…
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.

4 participants