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

Switch boost::shared_ptr to std::shared_ptr. #57

Merged
merged 1 commit into from Sep 27, 2016

Conversation

de-vri-es
Copy link
Contributor

This PR swaps out boost::shared_ptr for std::shared_ptr and one occasion of boost::scoped_ptr with std::unique_ptr (the latter one was internal, not part of the API).

geometric_shapes was already using std::shared_ptr for octrees for compatibility with fcl 0.5, with this PR everything will be the same shared_ptr type.

The C++11 check is also added to bodies.h header, since it also requires C++11 with this PR.

@v4hn
Copy link
Contributor

v4hn commented Sep 22, 2016

This arose out of moveit/moveit#215 (comment) and following.

The only question I have about this is whether this breaks collada_urdf (the only public inverse dependency of geometric_shapes) again.
Actually, I consider this to be part of 2506fa2 ,
but we had a release in between already, so it's not that easy...
A number of us should probably agree on this to change the API yet again if we want to streamline the use of shared_ptr in this package.

@de-vri-es
Copy link
Contributor Author

Note that CI seems to be failing because moveit is also getting build, but still using boost::shared_ptr typedefs that conflict with the ones from geometric_shapes.

Regarding collada_urdf, it build fine locally with these modification. Grepping through the source showed no explicit boost::shared_ptr<shapes:: and noboost::make_shared. So it seems to be safe.

@davetcoleman
Copy link
Member

+1 switching everything to C++11 at once makes sense to me. @de-vri-es is right the Travis build is fine for geometric_shapes, though @de-vri-es did you run the geometric_shapes tests manually to ensure those are ok?

If built with moveit/moveit#215 will everything pass?

@de-vri-es
Copy link
Contributor Author

@davetcoleman Locally I can build geometric_shapes with this PR, and all the tests succeed.

For moveit/moveit#215 some more changes are needed to moveit. I got it building locally, but since it depends on this PR it makes sense to decide on this one before updating the other (merge can be done at the same time).

@v4hn
Copy link
Contributor

v4hn commented Sep 23, 2016

ok, +1 to this request as is

@davetcoleman
Copy link
Member

Looks like we all agree on this PR, but it should not be merged until @de-vri-es has moveit/moveit#215 ready to merge with it

@de-vri-es
Copy link
Contributor Author

moveit/moveit#215 is updated for this PR (but there might be more to do there, see comments there).

@davetcoleman davetcoleman merged commit 456ea8d into moveit:kinetic-devel Sep 27, 2016
@davetcoleman
Copy link
Member

Thanks @de-vri-es !

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