Skip to content

[C++11/C++14] remove explicit -std=c++11#94

Closed
moriarty wants to merge 3 commits intoros:kinetic-develfrom
moriarty:use-default-std-cpp
Closed

[C++11/C++14] remove explicit -std=c++11#94
moriarty wants to merge 3 commits intoros:kinetic-develfrom
moriarty:use-default-std-cpp

Conversation

@moriarty
Copy link
Copy Markdown

From REP-0003

Melodic
As of ROS Melodic, we are using the C++14 (ISO/IEC 14882:2014) standard.

Related to discussion on moveit/moveit#1146

This should work with both Kinetic & Melodic:

For Kinetic, on 16.04 the default is gcc-5, which will use -std=c++11
https://www.gnu.org/software/gcc/gcc-5/changes.html

For Kinetc the other target platform was 15.10 reached EOL on July 28, 2016.
https://wiki.ubuntu.com/Releases

From REP-0003

> Melodic
> As of ROS Melodic, we are using the C++14 (ISO/IEC 14882:2014) standard.

Related to discussion on moveit/moveit#1146

This should work with both Kinetic & Melodic:

For Kinetic, on 16.04 the default is gcc-5, which will use -std=c++11
https://www.gnu.org/software/gcc/gcc-5/changes.html

For Kinetc the other target platform was 15.10 reached EOL on July 28, 2016.
https://wiki.ubuntu.com/Releases
@moriarty
Copy link
Copy Markdown
Author

😞 it looks like I read https://www.gnu.org/software/gcc/gcc-5/changes.html to quickly...
They bumped the default C standard, not C++ standard

The default mode for C is now -std=gnu11 instead of -std=gnu89.

@moriarty
Copy link
Copy Markdown
Author

@dirk-thomas

Is Lunar Loggerhead only supported on Xenial?

This is only for a test, so I don't think it really matters much. But I was looking for a way to use the same single kinetic-devel branch, but use C++14 for Melodic, else use C++11...

The hackish solution would be to check for C++17 support, because if you check for C++14 support it's true for both... But this only works if Lunar Loggerhead is only supported on Xenial and not still supported on Yakkety and Zesty, because gcc-6 has C++17.

@@ -38,7 +38,6 @@ if(CATKIN_ENABLE_TESTING)
   endif()
   catkin_add_gtest(${PROJECT_NAME}-test_time test/time.cpp)
   if(TARGET ${PROJECT_NAME}-test_time)
+    include(CheckCXXCompilerFlag)
+    CHECK_CXX_COMPILER_FLAG("-std=c++17" COMPILER_SUPPORTS_CXX17)
+    CHECK_CXX_COMPILER_FLAG("-std=c++11" COMPILER_SUPPORTS_CXX11)
+    if(COMPILER_SUPPORTS_CXX17)
+      set_property(TARGET ${PROJECT_NAME}-test_time APPEND_STRING PROPERTY COMPILE_FLAGS "-std=c++14")
+    elseif(COMPILER_SUPPORTS_CXX11)
-    set_property(TARGET ${PROJECT_NAME}-test_time APPEND_STRING PROPERTY COMPILE_FLAGS "-std=c++11")
+      set_property(TARGET ${PROJECT_NAME}-test_time APPEND_STRING PROPERTY COMPILE_FLAGS "-std=c++11")
+    endif()

     target_link_libraries(${PROJECT_NAME}-test_time ${catkin_LIBRARIES} rostime)
   endif()
 endif()

@moriarty
Copy link
Copy Markdown
Author

I just saw the most recent commit in this repo has a nicer way.
Use CHECK_CXX_SOURCE_COMPILES( to check if a C++11 feature is available, if it does not compile then set -std=c++11 otherwise don't explicitly set it.

@dirk-thomas
Copy link
Copy Markdown
Member

What is the purpose of removing the C++11 option? Just because Melodic could leverage C++14 doesn't mean it has to.

@moriarty
Copy link
Copy Markdown
Author

For Clarity and consistency, I gave a longer answer here:

ros/ros_comm#1525 (comment)

@dirk-thomas
Copy link
Copy Markdown
Member

Since you mentioned in ros/ros_comm#1525 (comment) that there is no technical reason to change this I will close this PR since the extra effort to check the supported compiler version (since kinetic-devel is used for multiple ROS distros) has no advantage.

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.

2 participants