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

Pass callbacks as universal references #147

Merged
merged 1 commit into from
Nov 6, 2015
Merged

Conversation

esteve
Copy link
Member

@esteve esteve commented Nov 5, 2015

This PR uses universal references for callbacks.

@esteve esteve added the in progress Actively being worked on (Kanban column) label Nov 5, 2015
@wjwwood
Copy link
Member

wjwwood commented Nov 5, 2015

Again, I appreciate it if this could be deferred until #140 is merged.

@esteve esteve force-pushed the perfect-forwarding branch from 50f2f2c to bdd1fd0 Compare November 5, 2015 22:16
@esteve
Copy link
Member Author

esteve commented Nov 5, 2015

Sure, no problem, it's not marked for review yet, though.

@esteve
Copy link
Member Author

esteve commented Nov 5, 2015

I've cancelled the CI jobs.

@esteve esteve force-pushed the perfect-forwarding branch from bdd1fd0 to 8c47f61 Compare November 6, 2015 17:49
@esteve esteve self-assigned this Nov 6, 2015
@esteve esteve added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Nov 6, 2015
@dirk-thomas
Copy link
Member

The Windows build will fail since you can only build with a single rmw impl. enabled.

@esteve
Copy link
Member Author

esteve commented Nov 6, 2015

I triggered two more jobs, one for OpenSplice and another for Connext:

http://ci.ros2.org/job/ros2_batch_ci_windows/590/
http://ci.ros2.org/job/ros2_batch_ci_windows/591/

@dirk-thomas
Copy link
Member

The "Connext" job will fail too since you must select a single rmw implementation.

@esteve
Copy link
Member Author

esteve commented Nov 6, 2015

@jacquelinekay
Copy link
Contributor

Can you give a summary of why a universal reference is needed, instead of the current behavior, or instead of an lvalue reference?

@esteve
Copy link
Member Author

esteve commented Nov 6, 2015

This way we avoid extra copies and passing normal references breaks lambdas.

@jacquelinekay
Copy link
Contributor

so you can't pass a lambda to a function by lvalue reference? I couldn't find any documentation on the matter.

I recall you made this PR because of that problem: #143

was it because somewhere the lambdas were being passed as temporaries? Then I can see why perfect forwarding would be needed.

Anyway, regardless of the details, the change is a good idea; I'm just trying to learn more about C++11...

@esteve
Copy link
Member Author

esteve commented Nov 6, 2015

Yes, the result of a lambda expression is a temporary, which can't be bound to lvalue references, hence the need for universal ones.

@esteve
Copy link
Member Author

esteve commented Nov 6, 2015

In general, any function that takes a template argument should use a universal reference, unless we really need copies.

@jacquelinekay
Copy link
Contributor

+1

esteve added a commit that referenced this pull request Nov 6, 2015
Pass callbacks as universal references
@esteve esteve merged commit f4a094a into master Nov 6, 2015
@esteve esteve removed the in review Waiting for review (Kanban column) label Nov 6, 2015
@esteve esteve deleted the perfect-forwarding branch November 6, 2015 20:04
nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022
mauropasse added a commit to mauropasse/rclcpp that referenced this pull request Aug 8, 2024
* Check if goal was sent through IPC before send responses
* Add intra_process_action_server_is_available API to intra-process Client


---------

Co-authored-by: Mauro Passerino <mpasserino@irobot.com>
mauropasse added a commit to mauropasse/rclcpp that referenced this pull request Aug 9, 2024
* Check if goal was sent through IPC before send responses
* Add intra_process_action_server_is_available API to intra-process Client


---------

Co-authored-by: Mauro Passerino <mpasserino@irobot.com>
mauropasse added a commit to mauropasse/rclcpp that referenced this pull request Aug 13, 2024
* Check if goal was sent through IPC before send responses
* Add intra_process_action_server_is_available API to intra-process Client


---------

Co-authored-by: Mauro Passerino <mpasserino@irobot.com>
mauropasse added a commit to mauropasse/rclcpp that referenced this pull request Aug 21, 2024
* Fixes for intra-process actions (ros2#144)

* Fixes for intra-process Actions

* Fixes for Clang builds

* Fix deadlock

* Server to store results until client requests them

* Fix feedback/result data race

See ros2#2451

* Add missing mutex

* Check return value of intra_process_action_send

---------

Co-authored-by: Mauro Passerino <mpasserino@irobot.com>

* Fix IPC Actions data race (ros2#147)

* Check if goal was sent through IPC before send responses
* Add intra_process_action_server_is_available API to intra-process Client


---------

Co-authored-by: Mauro Passerino <mpasserino@irobot.com>

* Fix data race in Actions: Part 2 (ros2#148)

* Fix data race in Actions: Part 2

* Fix warning - copy elision

---------

Co-authored-by: Mauro Passerino <mpasserino@irobot.com>

* fix: Fixed race condition in action server between is_ready and take"… (ros2#2531)

* fix: Fixed race condition in action server between is_ready and take" (ros2#2495)

Some background information: is_ready, take_data and execute data
may be called from different threads in any order. The code in the old
state expected them to be called in series, without interruption.
This lead to multiple race conditions, as the state of the pimpl objects
was altered by the three functions in a non thread safe way.

Co-authored-by: William Woodall <william@osrfoundation.org>
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

* fix: added workaround for call to double calls to take_data

This adds a workaround for a known bug in the executor in iron.

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

---------

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Co-authored-by: William Woodall <william@osrfoundation.org>

---------

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Co-authored-by: Mauro Passerino <mpasserino@irobot.com>
Co-authored-by: jmachowinski <jmachowinski@users.noreply.github.com>
Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Co-authored-by: William Woodall <william@osrfoundation.org>
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