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

Update std::shared_ptr argument passing to be closer to idiomatic C++. #1818

Merged

Conversation

jpieper-tri
Copy link
Contributor

The CppCoreGuidelines recommend that std::shared_ptr only be passed as
an argument when the callee explicitly needs to manage the lifetime of
the argument, and that 'const std::shared_ptr&' should only be used
if there is a chance that lifetime will not be shared.

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rr-smartptrparam

Many instances in drake were passing 'const std::shared_ptr&'
around, often when no lifetime management was necessary, and otherwise
when lifetime sharing was unconditional. This can be less efficient,
unnecessarily restrictive, and hides the intent of the API.

This patch converts those instances either to:

  • [const] T& if no lifetime sharing is required
  • std::shared_ptr if lifetime sharing is always required

@sherm1
Copy link
Member

sherm1 commented Mar 7, 2016

Some build failures, e.g.:

/tmp/drake-1797-9134/drake/systems/plants/constructModelmex.cpp:254:45: error: non-const lvalue reference to type 'RigidBody' cannot bind to a value of unrelated type 'shared_ptr<RigidBody>'
        model->addCollisionElement(element, b, group_name);
                                            ^
/tmp/drake-1797-9134/drake/../drake/systems/plants/RigidBodyTree.h:526:18: note: passing argument to parameter 'body' here
      RigidBody& body, const std::string& group_name);
                 ^

@jpieper-tri jpieper-tri force-pushed the no-const-shared-ptr-arg branch from a2fe71a to bdb4a36 Compare March 7, 2016 17:49
@jpieper-tri
Copy link
Contributor Author

sherm1: Thanks, hopefully fixed, although I'll have to rely on CI as I don't have matlab locally to test.

(other->parent.get() == this &&
!(other->joint && other->joint->isFloating())));
bool adjacentTo(const RigidBody& other) const {
return ((parent.get() == &other && !(joint && joint->isFloating())) ||
Copy link
Member

Choose a reason for hiding this comment

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

This and some of the nearby methods would be clearer with a utility method something like

return (parent->isSameRigidBody(other) && ...);

@sherm1
Copy link
Member

sherm1 commented Mar 7, 2016

Looks good to me, pending builds.

@jpieper-tri
Copy link
Contributor Author

Well, the windows jenkins build failed, and while the build output is full of warnings, I think the actual failure was just #1787 ?

@sherm1
Copy link
Member

sherm1 commented Mar 7, 2016

the windows jenkins build failed ... I think the actual failure was just #1787?

Yeah, the patch command is missing.

@jpieper-tri
Copy link
Contributor Author

Updated to fix merge conflicts.

@jpieper-tri
Copy link
Contributor Author

Does anyone know why jenkins failed here? It looks like it was trying to pull the incorrect git hash?

@jhoare
Copy link

jhoare commented Mar 8, 2016

retest this please

@jpieper-tri
Copy link
Contributor Author

Great, I believe the spurious Windows jenkins failures are now resolved.

@RussTedrake
Copy link
Contributor

@jamiesnape -- why did this pass on jenkins (but not appveyor)? it should not have passed without #1843 being merged.

@jamiesnape
Copy link
Contributor

@billhoffman added an environment variable specifying the generator for now.

@jamiesnape
Copy link
Contributor

(in the Jenkins build script, that is)

@RussTedrake
Copy link
Contributor

that doesn't explain why the pendulum urdf test passed.

The CppCoreGuidelines recommend that std::shared_ptr only be passed as
an argument when the callee explicitly needs to manage the lifetime of
the argument, and that 'const std::shared_ptr<T>&' should only be used
if there is a chance that lifetime will not be shared.

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rr-smartptrparam

Many instances in drake were passing 'const std::shared_ptr<T>&'
around, often when no lifetime management was necessary, and otherwise
when lifetime sharing was unconditional.  This can be less efficient,
unnecessarily restrictive, and hides the intent of the API.

This patch converts those instances either to:
 * [const] T& if no lifetime sharing is required
 * std::shared_ptr<T> if lifetime sharing is always required
@jpieper-tri jpieper-tri force-pushed the no-const-shared-ptr-arg branch from 16d0c77 to 82ecb83 Compare March 11, 2016 14:16
RussTedrake added a commit that referenced this pull request Mar 11, 2016
Update std::shared_ptr argument passing to be closer to idiomatic C++.
@RussTedrake RussTedrake merged commit b13ed3e into RobotLocomotion:master Mar 11, 2016
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.

5 participants