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 c++11 and remove boost dependency #62

Closed
scpeters opened this issue Aug 1, 2015 · 9 comments
Closed

Use c++11 and remove boost dependency #62

scpeters opened this issue Aug 1, 2015 · 9 comments

Comments

@scpeters
Copy link
Contributor

scpeters commented Aug 1, 2015

Are there any plans to make a version of urdfdom that uses c++11 and replaces boost::*_ptr with std::*_ptr? One semi-migration strategy could be to start using typedefs:

typedef boost::shared_ptr<urdf::Link> UrdfLinkPtr;

and then in the next major version of urdfdom switch them over to std:: instead of boost::

typedef std::shared_ptr<urdf::Link> UrdfLinkPtr;

Then anyone who wants their code to support both versions could use the typedefs? I'm sure there's big holes in this strategy (such as boost::*_pointer_cast vs std::*_pointer_cast), but you've got to start somewhere.

@scpeters
Copy link
Contributor Author

scpeters commented Aug 2, 2015

@wjwwood any thoughts?

@wjwwood
Copy link
Member

wjwwood commented Aug 2, 2015

We don't have a plan for migrating to C++11 types within ROS 1, but adding a typedef which doesn't already exist which points to the C++11 types is probably a good strategy. I'm not sure what the sticking points will be in the transition, but if I see a concrete proposal (pull request) I can give feedback on what might be affected based on that.

We already guarantee that ROS 1 will compile when -std=c++11 is passed to the compiler, so we only have to consider the upgrade with respect to types that have changed. We probably won't be able to drop the boost dependency anytime soon. Because we use boost::filesystem and boost::program_options and a few other things which have no C++11 equivalent.

Quickly looking at urdfdom it uses boost::lexical_cast:

#include <boost/lexical_cast.hpp>

Which is probably replaced by std::to_string.

It also uses shared_ptr in the public API:

URDFDOM_DLLAPI boost::shared_ptr<ModelInterface> parseURDF(const std::string &xml_string);

https://github.com/ros/urdfdom_headers/blob/56471a55d85e1be27a91a5f2f72b001d92cddb15/urdf_sensor/include/urdf_sensor/sensor.h#L157
(and others)

And weak_ptr:

https://github.com/ros/urdfdom_headers/blob/56471a55d85e1be27a91a5f2f72b001d92cddb15/urdf_model/include/urdf_model/link.h#L239

@isucan
Copy link
Contributor

isucan commented Aug 3, 2015

my concern is that there is a lot of code that depends on these types, so I am not too inclined to change this. What do you think of forking to an urdfdom2 that uses c++11 and avoids boost?

@scpeters
Copy link
Contributor Author

scpeters commented Aug 5, 2015

Adding c++11 is a huge change, so I had assumed that would involve a major version bump. I had imagined that it could just live in a new branch. Do we need to fork and make a package with a new name (like urdfdom2) for this type of change?

@scpeters
Copy link
Contributor Author

Bump my previous comment: can we make breaking changes in a new release branch (bumping the major version), or does it need to be in a new repository?

@wjwwood
Copy link
Member

wjwwood commented Sep 14, 2015

I'd say do it as a branch, but @isucan should make the call.

@isucan
Copy link
Contributor

isucan commented Sep 14, 2015

Please do this in a new branch, no need for a new repo.

@scpeters
Copy link
Contributor Author

I just made a pull request to urdfdom_headers to create typedefs for smart pointers. It should preserve backwards compatibility but make it easier to migrate away from boost.

ros/urdfdom_headers#13

@scpeters
Copy link
Contributor Author

boost is gone!

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

No branches or pull requests

4 participants