Skip to content

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

Merged
davetcoleman merged 1 commit intomoveit:kinetic-develfrom
fizyr-forks:std-shared-ptr
Sep 27, 2016
Merged

Switch boost::shared_ptr to std::shared_ptr.#57
davetcoleman merged 1 commit intomoveit:kinetic-develfrom
fizyr-forks:std-shared-ptr

Conversation

@de-vri-es
Copy link
Copy Markdown
Contributor

@de-vri-es de-vri-es commented Sep 22, 2016

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.

@de-vri-es de-vri-es mentioned this pull request Sep 22, 2016
Merged
@v4hn
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

de-vri-es commented Sep 22, 2016

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
Copy Markdown
Member

davetcoleman commented Sep 22, 2016

+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
Copy Markdown
Contributor Author

de-vri-es commented Sep 23, 2016

@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
Copy Markdown
Contributor

v4hn commented Sep 23, 2016

ok, +1 to this request as is

@davetcoleman
Copy link
Copy Markdown
Member

davetcoleman commented Sep 23, 2016

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
Copy Markdown
Contributor Author

de-vri-es commented Sep 24, 2016

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
Copy Markdown
Member

davetcoleman commented Sep 27, 2016

Thanks @de-vri-es !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davetcoleman davetcoleman davetcoleman approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@de-vri-es @v4hn @davetcoleman