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

PerformAction Event Description #49

Merged
merged 6 commits into from
Jan 20, 2022
Merged

PerformAction Event Description #49

merged 6 commits into from
Jan 20, 2022

Conversation

Yadunund
Copy link
Member

Cherry picked the description introduced here #46

@mxgrey mxgrey self-requested a review January 20, 2022 06:08
Copy link
Contributor

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

This looks really great! I just left a few nitpicks, but none of them are blockers.

auto PerformAction::Description::use_tool_sink(
bool use_tool) -> Description&
{
_pimpl->use_tool_sink = std::move(use_tool);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really important, but just a remark: Moving a primitive type like bool doesn't really do anything special compared to copying it. Either way it will perform a copy.

Moving only matters when one or more fields are heap-allocated objects whose copy operators perform a deep copy, like std::vector or std::[unordered_]map. Or objects whose copy operator has side effects, like std::shared_ptr's copy operation increments a reference count while its move operation doesn't. Or most importantly objects that don't offer a copy operator like std::unique_ptr.

Using move here doesn't hurt anything, but I thought it would be good to share the above info.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the explanation; very helpful 👍🏼

rmf_task_sequence/src/rmf_task_sequence/events/utils.hpp Outdated Show resolved Hide resolved
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
@Yadunund Yadunund merged commit c4d17fb into redesign_v2 Jan 20, 2022
@Yadunund Yadunund deleted the perform_action branch January 20, 2022 08:49
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.

2 participants