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

use default copy constructor to clone attached object #2855

Merged

Conversation

simonschmeisser
Copy link
Contributor

I stumbled upon this when helping @felixvd debug #2854 ... this PR would have prevented his long despair

C++ provides a default copy constructor if it reasonably can. AttachedBody is a simple data structure so indeed it can here. Using the copy constructor prevents forgetting to manually copy some member variables.

Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

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

attachBody(T*) is documented to assume ownership, so the new is fine.
I'm wondering whether it wouldn't be nicer to create a new attachBody(const T&) overload instead that copies the object internally to hide memory management.

@@ -177,9 +177,7 @@ void RobotState::copyFrom(const RobotState& other)
// copy attached bodies
clearAttachedBodies();
for (const std::pair<const std::string, AttachedBody*>& it : other.attached_body_map_)
attachBody(it.second->getName(), it.second->getPose(), it.second->getShapes(), it.second->getShapePoses(),
it.second->getTouchLinks(), it.second->getAttachedLinkName(), it.second->getDetachPosture(),
it.second->getSubframes());
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes.

@codecov
Copy link

codecov bot commented Aug 28, 2021

Codecov Report

Merging #2855 (4d66106) into master (3c22249) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2855      +/-   ##
==========================================
+ Coverage   60.85%   60.86%   +0.02%     
==========================================
  Files         366      366              
  Lines       31712    31710       -2     
==========================================
+ Hits        19296    19298       +2     
+ Misses      12416    12412       -4     
Impacted Files Coverage Δ
moveit_core/robot_state/src/robot_state.cpp 51.56% <100.00%> (-0.08%) ⬇️
...ipulation/pick_place/src/manipulation_pipeline.cpp 69.45% <0.00%> (-1.85%) ⬇️
...g_scene_interface/src/planning_scene_interface.cpp 48.59% <0.00%> (+1.13%) ⬆️
...meterization/work_space/pose_model_state_space.cpp 83.02% <0.00%> (+2.52%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c22249...4d66106. Read the comment docs.

@rhaschke rhaschke merged commit 4f38ef8 into moveit:master Aug 29, 2021
@simonschmeisser simonschmeisser deleted the attached-body-use-copy-constructor branch September 2, 2021 14:26
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

3 participants