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

Corrected publish calls with shared_ptr signature #327

Merged
merged 7 commits into from
May 6, 2019

Conversation

ivanpauno
Copy link
Member

@ivanpauno ivanpauno commented Apr 29, 2019

Connects to ros2/rclcpp#709

@ivanpauno ivanpauno self-assigned this Apr 29, 2019
@ivanpauno ivanpauno added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 29, 2019
@dirk-thomas
Copy link
Member

In many cases same comment as here: ros2/rclcpp#709 (comment)

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

I started commenting on each, but I think most, if not all, can be converted to use a reference rather than a pointer (unique or shared).

Can you please look at each case and try to convert to a reference, and failing that convert to a unique_ptr, and failing that leave it as a shared_ptr. I'll have a look at the cases for the latter two results, because I expect them to be limited, and the shared_ptr to be unnecessary in all cases.

composition/src/talker_component.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp/src/topics/allocator_tutorial.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp/src/topics/talker.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp_native/src/talker.cpp Outdated Show resolved Hide resolved
dummy_robot/dummy_map_server/src/dummy_map_server.cpp Outdated Show resolved Hide resolved
@wjwwood
Copy link
Member

wjwwood commented Apr 29, 2019

Actually, should we prefer unique_ptr? In the case that intra-process is enabled, it might be better to prefer unique_ptr, but if you're only doing inter-process then the stack reference is probably preferable.

Thoughts?

@ivanpauno ivanpauno force-pushed the ivanpauno/deprecate-shared-ptr-publish branch from 04ff47f to ced1a6a Compare May 2, 2019 14:41
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno force-pushed the ivanpauno/deprecate-shared-ptr-publish branch from e14b1d8 to 94f88aa Compare May 2, 2019 17:13
Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

I'm not familiar enough with the intra-process changes to comment on whether stack variables vs unique_ptr should be used. The changes that are here seem reasonable

size_t i = 1;

// Our main event loop will spin until the user presses CTRL-C to exit.
while (rclcpp::ok()) {
// Initialize a shared pointer to an Image message.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, shared -> unique

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@wjwwood wjwwood merged commit 1a4dc3c into master May 6, 2019
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/deprecate-shared-ptr-publish branch May 6, 2019 20:33
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label May 6, 2019
routiful pushed a commit to ROBOTIS-move/demos that referenced this pull request May 7, 2019
* Corrected publish calls with shared_ptr signature

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

* Corrected with PR comments

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

* Changed unique_ptr publish calls

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

* Corrected publish call with unique_ptr in logger_usage_component

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

* Solve linter problems. Corrected pendulum_control demo.

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

* Cleaned allocator_tutorial

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

* Corrected talker_serialized_message.cpp

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: Darby Lim <thlim@robotis.com>
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.

None yet

5 participants