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 for moveit plugin when available #254

Closed

Conversation

jonbinney
Copy link
Contributor

This is needed to compile on kinetic. Otherwise you get shared_ptr errors in geometric_shapes headers, which get included by moveit headers. I've only tested this on xenial/kinetic; can someone make sure I haven't broken compile on indigo?

@de-vri-es
Copy link
Contributor

de-vri-es commented Jul 29, 2016

I think it shouldn't break indigo, though it will cause compiling with C++11 enabled even for indigo if the compiler supports it (which is the case in Ubuntu 14.04). That shouldn't cause any problems as long as it's still ABI compatible, which I think should be the case.

GCC 5.1 did introduce a new C++11 ABI, but whether or not that is used is independent of actually compiling with or without C++11 support. [1]

We have actually been mixing C++11 and non C++11 objects with indigo for quite some time. The only problems we encountered were with the Ubuntu packaged version of PCL, so there should be no problems here.

Bottom line: Looks good to me :)

[1] https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_dual_abi.html

@de-vri-es
Copy link
Contributor

de-vri-es commented Jul 29, 2016

I suppose an alternative is to simple add add_compile_options(-std=c++11) on the kinetic branch. C++11 support is a requirement for kinetic, so in that case no check is needed.

However, there is something to be said for using the same code base across the branches as long as possible.

/edit: And of course, it is neater if the configure step detects lack of C++11 support rather than the compile step. However, most packages so far simply add the compile flags without checking.

@jonbinney
Copy link
Contributor Author

Are there any other reasons to create a new branch for kinetic? If there are then I'd rather just put this fix into a kinetic branch. I think that moveit is close to being released, which was one of the last blockers for releasing this repo into kinetic (#240) so if a kinetic-devel branch is going to be created now would be the time.

@jonbinney
Copy link
Contributor Author

It turns out that this should be fixed in geometric_shapes - that package shouldn't be using C++11 features in its API, and if it does, it should export the appropriate flags. There is discussion about how to fix it in a few places:

But however that gets resolved, the code in this repo shouldn't have to change. Closing this PR.

@jonbinney jonbinney closed this Aug 2, 2016
@jonbinney
Copy link
Contributor Author

After much discussion, it was decided that each package which uses geometric_shapes will need to enable c++11 on its own: http://discourse.ros.org/t/std-shared-ptr-in-the-geometric-shapes-public-api/395/10

So I'm re-opening this.

@davetcoleman
Copy link
Contributor

Can we branch for kinetic (kinetic-devel) now for this repo? Seems like this C++11 flag is only needed for that use case.

@shaun-edwards
Copy link
Member

@davetcoleman, do you just want a kinetic branch OR do you want a kinect release. #281 is possibly an issue with Kinetic (although not a build issue).

@davetcoleman
Copy link
Contributor

davetcoleman commented Jan 4, 2017

I think there should be a kinetic-devel branch. I'm working with this repo now and am finding lots of warnings from deprecated things from MoveIt! in the *_moveit_config packages.

@davetcoleman
Copy link
Contributor

I'm attempting to standardize C++11 usage for ROS: ros-infrastructure/catkin_pkg#154

@shaun-edwards shaun-edwards changed the base branch from indigo-devel to kinetic-devel January 5, 2017 01:44
@shaun-edwards
Copy link
Member

@jonbinney, since @davetcoleman (and others) have suggested using add_definitions(-std=c++11) to enable C++11, can you make this change? I've updated the PR to point to the kinetic-devel branch so no need for the compiler check.

@jonbinney
Copy link
Contributor Author

@shaun-edwards will do.

@davetcoleman
Copy link
Contributor

@jonbinney
Copy link
Contributor Author

Ah, thanks @davetcoleman ! I'll close this PR then.

@jonbinney jonbinney closed this Jan 5, 2017
ipa-nhg pushed a commit to ipa-nhg/universal_robot that referenced this pull request Jul 2, 2019
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.

4 participants