moveit / moveit_core

THIS REPO HAS MOVED TO https://github.com/ros-planning/moveit
31 stars 76 forks source link

std::shared_ptr versus boost::shared_ptr #319

Closed de-vri-es closed 8 years ago

de-vri-es commented 8 years ago

Since C++11 is now officially supported, that raises the issue on whether or not to phase out boost::shared_ptr for std::shared_ptr in the public API of moveit.

In terms of performance or availability I don't think there is any difference. Both boost and the standard library should have high quality shared_ptr implementations and should be available on pretty much every platform that could reasonably be targeted by ROS and moveit.

However, mixing the two types is relatively annoying for users. Knowing that a function accepts a shared_ptr suddenly isn't enough any more, you also need to know which shared_ptr. Additionally, on-the-fly conversion between the two is not possible without breaking weak_ptr in at least some situations [1]. (Be sure to read all comments for the full explanation, even the hidden ones).

So there is something to be said for switching everything to std::shared_ptr. In the future, use of boost::shared_ptr will almost certainly become rarer and rarer, while std::shared_ptr will be taking over instead.

However, external libraries can force the use of a specific type of shared_ptr. For example, FCL 0.5 now requires us to use std::shared_ptr in some places. Similarly, roscpp and with it many other ROS libraries are still using boost::shared_ptr in their API. Even worse, at some point two libraries might require different shared_ptr types for the same data.

I expect that one reason ROS isn't migrating yet is that boost::shared_ptr will also work without problem pre-kinetic. So switching to std::shared_ptr makes backport (or forward-porting ^_^) more complicated.

So, personally I'm on the fence on this issue. I prefer std::shared_ptr, but as long as roscpp is holding the whole of ROS on boost::shared_ptr, it might be wise (or at least pragmatic) to stick with that decision where possible.

[1] http://stackoverflow.com/questions/12314967/cohabitation-of-boostshared-ptr-and-stdshared-ptr/12315035#12315035

v4hn commented 8 years ago

Is there a discussion somewhere on roscpp still using boost::shared_ptr? As they require C++11 in kinetic, I expected they would move to std:: where possible.

Mixing shared pointer types will really be quite painful, but as roscpp for kinetic is already released, I don't see a way around that.. This will haunt us until 2021 at least. -.-

Thanks for investigating!

davetcoleman commented 8 years ago

I'm curious where in the MoveIt! code base we use roscpp code with boost::shared_ptr? Searching for boost::shared_ptr<ros I only get one trivial instance that is unimportant and can easily be fixed. I wonder if the mixing is actually a non-issue.

I would really like to see us migrate to std::shared_ptr so that this project is relevant now and in 2021. A good sign that we should switch is that FCL and Ocotomap have.

makes backport (or forward-porting ^_^) more complicated.

This is the only hold up I see since a number of our maintainers are still on indigo. But I suspect most of our non-breaking ABI/API changes won't introduce variables or functions, so the cherry-picks still might just work (a robotstate works the same except in function headers and in their class declaration).

de-vri-es commented 8 years ago

I was mainly thinking of all the automatically generated code for messages and services. Those interfaces rely heavily on boost::shared_ptr, although often they are used through typedefs (like sensor_msgs::JointStatePtr or sensor_msgs::JointStateConstPtr).

de-vri-es commented 8 years ago

Conversation should continue here: ros-planning/moveit#48.

Closing this issue.