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

if compiler supports it use C++11 #223

Merged

Conversation

bbferka
Copy link
Contributor

@bbferka bbferka commented May 22, 2018

added compilation flags for C++11

@furushchev
Copy link
Collaborator

@bbferka Thank you for sending a pull request! Just one question, is it possible to enable/disable c++11 features using something like if($ENV{ROS_DISTRO}) condition?
I think it'd be the best if it is possible to disable this feature on indigo.

@bbferka
Copy link
Contributor Author

bbferka commented May 22, 2018

@furushchev I guess that is also possible. Since indigo only works with Ubuntu 14.04, which comes with an old gcc, check_cxx_compiler_flag("-std=c++11" COMPILER_SUPPORTS_CXX11) would also simply result in building without c++11 for indigo; If you prefer the ROS_DISTRO solution I can change that.

@furushchev
Copy link
Collaborator

@bbferka Cool! I think it is enough just to add it as a comment to CMakeLists.txt that c++11 feature will be disabled in indigo.

@bbferka
Copy link
Contributor Author

bbferka commented May 22, 2018

@furushchev sorry, I was mistaken. gcc 4.8 already has C++11 support. I will change my code to use the ROS_ENV env. var., and disable C++11 for indigo

@hawesie
Copy link
Member

hawesie commented May 22, 2018

What about future releases? The current setup means we have to manually add each new ROS release that supports C++11. Wouldn't it be better to check the negative, e.g. if indigo then c++11 is disabled, else all other releases have it enabled?

@bbferka
Copy link
Contributor Author

bbferka commented May 23, 2018

@hawesie you're right. I just changed the check;

@hawesie
Copy link
Member

hawesie commented May 23, 2018

Tests pass. All is well. Thanks @bbferka

@hawesie hawesie merged commit d854e7c into strands-project:kinetic-devel May 23, 2018
@furushchev
Copy link
Collaborator

@bbferka @hawesie Thanks too for interating from me 👍

@furushchev
Copy link
Collaborator

@hawesie Could you release the new version including this change? I think it is not a small change, so recommend to increment a major or minor version (more than usual updates).

@hawesie
Copy link
Member

hawesie commented May 23, 2018

ok, it's on my list for today

@bbferka
Copy link
Contributor Author

bbferka commented May 23, 2018

@furushchev @hawesie thanks for merging!

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.

3 participants