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

[C++14] Use C++14 in Melodic #1146

Merged
merged 1 commit into from Nov 26, 2018

Conversation

Projects
None yet
10 participants
@moriarty
Copy link
Contributor

commented Oct 25, 2018

Description

On Ubuntu 18.04, gcc-7 defaults to C++14.
On Ubuntu 16.04, C++14 is available, but only used if explicitly set.

http://www.ros.org/reps/rep-0003.html#melodic-morenia-may-2018-may-2023

Targeted Languages:

  • C++14

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extended the tutorials / documentation, if necessary reference
  • Include a screenshot if changing a GUI
  • Document API changes relevant to the user in the moveit/MIGRATION.md notes
  • Created tests, which fail without this PR reference
  • Decide if this should be cherry-picked to other current ROS branches
  • While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers
[C++14] Use C++14 in Melodic
On Ubuntu 18.04, gcc-7 defaults to C++14.
On Ubuntu 16.04, C++14 is available, but only used if explicitly set.
@welcome

This comment has been minimized.

Copy link

commented Oct 25, 2018

Thanks for opening this pull request! Please check out our contributing guidelines.

@moriarty

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2018

@davetcoleman as discussed during World MoveIt Day. The local build is still running 😉 so wait for the Jenkins job

@davetcoleman
Copy link
Member

left a comment

Do not merge until we get approval from @v4hn and @rhaschke

@rhaschke

This comment has been minimized.

Copy link
Collaborator

commented Oct 25, 2018

I'm not sure about the side effects of this. I remember we had shared_ptr issues when mixing C++98 and C++11 compiled code in the past. What is the default used by basic ROS libs, e.g. roscpp, urdf, etc.?

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

@rhaschke

This comment has been minimized.

Copy link
Collaborator

commented Oct 25, 2018

Melodic is C++14

So, we will not cherry-pick this to Kinetic?

@moriarty

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2018

@rhaschke re: mixing C++98 and C++11, Those were particularly big changes in C++, but were they run-time or compile-time issues?

This shouldn't be cherry-picked to Kinetic

@wjwwood

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2018

My experience is that if you mix use of c++11 and c++14 you'll be ok if it's the same compiler and stdlib just with different settings (the compiler and standard library developers work hard ensure this ABI compatibility).

So 👍 from me, but it's not guaranteed to be issue free.

@moriarty

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2018

I'm now recompiling and will run a mixed test, directly on the robot hardware:

fetch at fetch3 in ~/melodic-stable/src
$ grep -r c++1 --include=CMakeLists.txt
fetch_ros/fetch_ikfast_plugin/CMakeLists.txt:  add_compile_options(-std=c++11)
moveit/moveit_plugins/moveit_controller_manager_example/CMakeLists.txt:add_compile_options(-std=c++14)
moveit/moveit_plugins/moveit_fake_controller_manager/CMakeLists.txt:add_compile_options(-std=c++14)
moveit/moveit_plugins/moveit_ros_control_interface/CMakeLists.txt:add_compile_options(-std=c++14)
moveit/moveit_plugins/moveit_simple_controller_manager/CMakeLists.txt:add_compile_options(-std=c++14)
moveit/moveit_setup_assistant/CMakeLists.txt:add_compile_options(-std=c++14)
moveit/moveit_experimental/CMakeLists.txt:add_definitions(-std=c++14)
...

fetch_ikfast_plugin with C++11, and this branch for MoveIt C++14...

@moriarty

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2018

Unfortunately we didn't have time to have get it compiled and tested on the robot @velveteenrobot, @jonbinney and I were ssh'd into the same robot, sharing a catkin workspace it got into a bit of a mess.

To ensure it was all the changes on the robot causing the catkin errors, I setup a clean environment on a server and tested that things compiled... And it worked... so it was something we had in our local environment, we can clean it up and test it on hardware tomorrow.

I was going to try out another robot in gazebo simulation... but there is no melodic-devel branch for moveit_robots: ros-planning/moveit_robots#71

@simonschmeisser

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

We have had MoveIt! Kinetic running on C++14 for some while as our/ensenso's profiling tool requires that ... no issues observed

@gavanderhoorn

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

@davetcoleman wrote:

Melodic is C++14
http://www.ros.org/reps/rep-0003.html#melodic-morenia-may-2018-may-2023

Bit late, but I don't think you can say this.

The language used in the REP is:

Targeted Languages:

  • C++14

but, if we look at how roscpp is compiled (for instance, there are many other packages that do similar things):

[ 33%] Building CXX object CMakeFiles/roscpp.dir/src/libros/master.cpp.o
/usr/bin/c++  -DROSCONSOLE_BACKEND_LOG4CXX -DROS_PACKAGE_NAME=\"roscpp\" -Droscpp_EXPORTS -I/tmp/binarydeb/ros-melodic-roscpp-1.14.3/obj-x86_64-linux-gnu/devel/include -I/tmp/binarydeb/ros-melodic-roscpp-1.14.3/include -I/tmp/binarydeb/ros-melodic-roscpp-1.14.3/obj-x86_64-linux-gnu/devel/include/ros -I/opt/ros/melodic/include -I/opt/ros/melodic/share/xmlrpcpp/cmake/../../../include/xmlrpcpp -I/tmp/binarydeb/ros-melodic-roscpp-1.14.3/obj-x86_64-linux-gnu  -g -O2 -fdebug-prefix-map=/tmp/binarydeb/ros-melodic-roscpp-1.14.3=. -fstack-protector-strong -Wformat -Werror=format-security -DNDEBUG -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC   -std=c++11 -Wall -Wextra -o CMakeFiles/roscpp.dir/src/libros/master.cpp.o -c /tmp/binarydeb/ros-melodic-roscpp-1.14.3/src/libros/master.cpp

Note the -std=c++11 in there.


Edit: C++11 explicitly set here.

@moriarty

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2018

@wjwwood see the comment above

Edit: C++11 explicitly set here.

Should/Can that change for Melodic?

Some of the other ROS-Comm stack packages have switched ros/class_loader@a2f4cca

The only ones which haven't are:

(in a clean rosinstall_generator ros_comm)

$ grep -r --include=CMakeLists.txt  "\-std" .
./ros_comm/xmlrpcpp/test/CMakeLists.txt:set_target_properties(test_base64 PROPERTIES COMPILE_FLAGS -std=c++11)
./ros_comm/roscpp/CMakeLists.txt:  set_directory_properties(PROPERTIES COMPILE_OPTIONS "-std=c++11;-Wall;-Wextra")
./pluginlib/CMakeLists.txt:  check_cxx_compiler_flag("-std=c++11" COMPILER_SUPPORTS_CXX11)
./pluginlib/CMakeLists.txt:      set_target_properties(${PROJECT_NAME}_unique_ptr_test PROPERTIES COMPILE_FLAGS -std=c++11 LINK_FLAGS -std=c++11)
./roscpp_core/rostime/CMakeLists.txt:    set_property(TARGET ${PROJECT_NAME}-test_time APPEND_STRING PROPERTY COMPILE_FLAGS "-std=c++11")

All the packages which aren't setting this flag will use the default C++14 set by gcc-7 in Ubuntu 18.04, an ls of this workspace is:

catkin
class_loader
cmake_modules
gencpp
geneus
genlisp
genmsg
gennodejs
genpy
message_generation
message_runtime
pluginlib
ros
ros_comm
ros_comm_msgs
rosconsole
roscpp_core
ros_environment
roslisp
rospack
std_msgs
@davetcoleman

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

Bit late, but I don't think you can say this.

@gavanderhoorn that's a misadvertisement from OSRF then that should be clarified / fixed in the REP or core ROS code

@gavanderhoorn

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

Bit late, but I don't think you can say this.

@gavanderhoorn that's a misadvertisement from OSRF then that should be clarified / fixed in the REP or core ROS code

Use of the word "targets" in REP-003 has been explained in the past as meaning: new / user code should make sure to be compatible with / use the C++ standard mentioned in the REP, but existing packages (such as roscpp) will not be updated to that particular standard.

As long as ABI compatibility is guaranteed between C++11 and C++14 then there would seem to be no problem, but I at least wanted to highlight the difference between "Melodic is C++14" and "C++14 code is allowed in Melodic".

@moriarty

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2018

@gavanderhoorn

Use of targets in REP-003 has been explained in the past as meaning

Yes, but I agree with @davetcoleman this is what is in the REP:

Melodic
As of ROS Melodic, we are using the C++14 (ISO/IEC 14882:2014) standard. All packages are free to use C++14 features in public and private APIs. Before changing existing public APIs to use C++14 features, package maintainers should carefully consider whether the change is worth the breakage to downstream consumers of the API. If at all possible, maintainers should use a "tick-tock" model where the new APIs are introduced alongside the old (deprecated) APIs, and then remove the old APIs in a subsequent release.

It doesn't say targeting, it says using C++14, which seems like some were just forgotten.

moriarty added a commit to moriarty/ros_comm that referenced this pull request Oct 26, 2018

[C++14] remove explicit -std=c++11, default to 14
From REP-0003
> Melodic
> As of ROS Melodic, we are using the C++14 (ISO/IEC 14882:2014) standard.

Related to discussion on ros-planning/moveit#1146

moriarty added a commit to moriarty/pluginlib that referenced this pull request Oct 26, 2018

[C++14] remove explicit -std=c++11 and checks
From REP-0003

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

Related to discussion on: ros-planning/moveit#1146
Similar to: ros/class_loader#87

moriarty added a commit to moriarty/roscpp_core that referenced this pull request Oct 26, 2018

[C++11/C++14] remove explicit -std=c++11
From REP-0003

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

Related to discussion on ros-planning/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
@gavanderhoorn

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

@moriarty: I'm not sure that is the case:

All packages are free to use C++14 features in public and private APIs.

nowhere does it state that ROS Melodic uses C++14 everywhere, nor that it is a requirement.

It doesn't say targeting, it says using C++14, which seems like some were just forgotten.

and the REP does use the word target, see my earlier comment (#1146 (comment)).

@wjwwood

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

Sorry, I’m traveling today so I can’t dig into the history to find the discussion about it. For now I’ll just cc @dirk-thomas since he’s the maintainer of ros_comm and @clalancette because he’s the melodic ros boss.

@moriarty

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2018

I opened the PRs to the packages from the ros_comm stacks, some tests failed to find std::nextafter and I'll look into those.

nowhere does it state that ROS Melodic uses C++14 everywhere, nor that it is a requirement.

@gavanderhoorn
This line from the REP needs editing if it is not the case. It doesn't say everywhere but it does say we are using.

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

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

If the ros_comm packages aren't using C++14 and then this same issue of C++11/C++14 compatibility is going to be brought up and discussed with any downstream package in the ecosystem... which would be a bit waste of time: C++17 is already out: and we're eagerly awaiting C++20...
Kinetic on Ubuntu 16.04 is available until May 2021 for those who need C++11.

@gavanderhoorn

This comment has been minimized.

Copy link
Member

commented Oct 27, 2018

If the ros_comm packages aren't using C++14 and then this same issue of C++11/C++14 compatibility is going to be brought up and discussed with any downstream package in the ecosystem...

you seem to mistake me for someone who came up with this policy / approach. I'm not and I wasn't.

I just commented here to clarify that @davetcoleman's assertion was essentially not true, and then I explained how "targets" has been explained in the past (when C++11 first appeared in REP-3) as that seemed to have caused some confusion.

Do you expect everyone to update all their packages to C++14 (if needed) in order to release into Melodic? That is both infeasible as well as unmaintainable.

In any case: let's see whether @dirk-thomas comments.

@moriarty

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2018

@gavanderhoorn

Sorry, the above comment wasn’t meant to be directed at you. But in general, and maybe the Open Robotics maintainers who have been tagged or anyone who follows the links from the PRs I opened.

You brought up a valid point, which I would summarize as:
“do we need to be concerned with C++11/C++14 compatibility.”

And in my opinion, from reading the REP the answer should be no. All packages compiled with GCC 6+ which are not explicity setting C++11, it’s defaulting to C++14.

And to be honest: because most of this stemmed from a face to face conversation at move it day with @wjwwood & others...

The main reason I’m pushing for this so strongly, isn’t even to get any particular C++14 features in this release... It is so that we are one step closer towards getting C++17 features in a near future release.

So again: my apologies if the comments felt directed at you specifically.

@clalancette

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2018

So, I originally wrote the update to REP-0003 for Melodic. And looking at it again, I agree that the wording is somewhat ambiguous. @gavanderhoorn interpretation is correct; packages are free to use C++14 in Melodic, but are not required to do so. I'd happily review and merge any changes to REP-0003 that makes this less ambiguous.

@moriarty

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

@clalancette how do you suggest to change the wording?

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

I think that would clarify the state of things. The sentence which follows is clear:

All packages are free to use C++14 features in public and private APIs. Before changing existing public APIs to use C++14 features, package maintainers should carefully consider whether the change is worth the breakage to downstream consumers of the API.

But still lead to confusion as if it's safe to use C++14 when there are core libraries are setting -std=c++11 ... This is done in only 4 places, so ros/pluginlib#131, ros/roscpp_core#94, and ros/ros_comm#1525

@clalancette

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2018

@clalancette how do you suggest to change the wording?

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

That wording is fine, though I slightly prefer:

-  As of ROS Melodic, we are using the C++14 (ISO/IEC 14882:2014) standard.
+  As of ROS Melodic, we default to using the C++14 (ISO/IEC 14882:2014) standard, but not all packages are required to do so.

But still lead to confusion as if it's safe to use C++14 when there are core libraries are setting -std=c++11 ... This is done in only 4 places, so ros/pluginlib#131, ros/roscpp_core#94, and ros/ros_comm#1525

Yeah, that's a separate issue, which will be resolved one way or another in ros/ros_comm#1525

dirk-thomas added a commit to ros/ros_comm that referenced this pull request Oct 31, 2018

[C++14] remove explicit -std=c++11, default to 14 (#1525)
From REP-0003
> Melodic
> As of ROS Melodic, we are using the C++14 (ISO/IEC 14882:2014) standard.

Related to discussion on ros-planning/moveit#1146
@moriarty

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

@davetcoleman wrote:

Do not merge until we get approval from @v4hn and @rhaschke

ros/ros_comm#1525 has been merged.

@dirk-thomas

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

ros/ros_comm#1525 has been merged.

And ros/roscpp_core#94 has been rejected - just to provide the full context.

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

still +1 from me :)
I think the only risk is maybe it would affect Ubuntu 16.04 users building the melodic branch, but seems unlikely.

@moriarty

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2018

@davetcoleman actually I had thought of that, and it is why the change sets the version to c++14 instead of removing the option and letting it us the default from gcc.

-  add_compile_options(-std=c++11)
+  add_compile_options(-std=c++14)

Ubuntu 16.04 and ROS Kinetic ship with gcc (Ubuntu 5.4.0-6ubuntu1~16.04.10) 5.4.0 20160609
C++14 is available on gcc-5, but it wasn't the default option until gcc-6.
So, this should work for users building on 16.04, but Kinetic doesn't support it as per REP-0003, and Melodic doesn't support 16.04 so that does seem unlikely.

@dirk-thomas
Yes, that ros/roscpp_core#94 was rejected, but it was actually only effecting the unit test. So if you compile roscpp_core without CATKIN_ENABLE_TESTING it will use c++14 as per the gcc default.

@rhaschke rhaschke referenced this pull request Nov 26, 2018

Closed

201811 release #1225

11 of 12 tasks complete

@v4hn v4hn merged commit f802133 into ros-planning:melodic-devel Nov 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@welcome

This comment has been minimized.

Copy link

commented Nov 26, 2018

Congrats on merging your first pull request and contributing to open source robotics!

@gavanderhoorn

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

Congrats on merging your first pull request and contributing to open source robotics!

lol.

Who is this bot addressing with this comment? :) @moriarty, or @v4hn?

In both cases it's funny :)

@v4hn

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

Congrats on merging your first pull request and contributing to open source robotics!

@davetcoleman you don't mean that comment, do you? That's just ridiculous.
If it is meant to address @moriarty , it should probably say "on getting your first pull request merged" instead.

On the pull-request: there is nothing wrong with C++14 on 16.04. I'm glad to get the additional expressiveness, although I really hope we need almost none of it.

Two things we might want to consider:

  • [[deprecated]] instead of our MOVEIT_DEPRECATED macro - the earlier allows to add comments without unnecessary bloat.

  • std::make_unique

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

lol. I'm happy to tweak the language but I really feel like overall its an "automated" way to improve inclusivity. From opensource.com:

Inclusivity is the quality of an open organization that allows and encourages people to join the organization and feel a connection to it. Practices aimed at enhancing inclusivity are typically those that welcome new participants to the organization and create an environment that makes them want to stay.

@moriarty

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

Awesome! I guess it’s about time I made a PR to MoveIt!, I’ve been using it for years.

I remember seeing some other places where I think boost was used but can be replaced.

run-clang-tidy -header-filter='.*' -checks='-*,modernize-*'

https://clang.llvm.org/extra/clang-tidy/checks/list.html

@v4hn

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

Awesome! I guess it’s about time I made a PR to MoveIt!, I’ve been using it for years.

🥇 Please take this as positive feedback to contribute more in the future 😃

I remember seeing some other places where I think boost was used but can be replaced.

Please feel free to look into it.

@VictorLamoine

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

I'm afraid the way C++14 is enabled is not right, not only for MoveIt but for every ROS package (that uses a specific C++ standard) I've seen so far.

I've already seen some discussions about how C++14 should be triggered, here is a quick summary about the different ways of enabling a specific C++ standard:

  • add_compile_options
  • add_definitions
  • CMAKE_CXX_STANDARD (and CMAKE_CXX_STANDARD_REQUIRED, CMAKE_CXX_EXTENSIONS)

There are others like target_compile_options but it would not make a lot of sense to enable C++11 only for a subset of each project.

The only one that is safe is CMAKE_CXX_STANDARD because it will "overwrite" add_compile_options and add_definitions.

Example:

  • You specify a C++ standard (C++14) using add_compile_options
  • You find package and link to a project that defines CMAKE_CXX_STANDARD to C++11

You will end up with a compilation line that looks like:
-std=c++14 -Wall -Wextra -fPIC -std=gnu++11

The latest flag is the one that will prevail, resulting in C++11 code! (full example of this problem here).

The CMAKE_CXX_STANDARD variable was added in CMake 3.1, while ROS packages usually only require CMake 2.8.3 (first version that catkin supports).

Should I make a pull request so that packages requires CMake 3.1 and enables C++14 properly?

tahsinkose added a commit to tahsinkose/ros_comm that referenced this pull request Apr 15, 2019

[C++14] remove explicit -std=c++11, default to 14 (ros#1525)
From REP-0003
> Melodic
> As of ROS Melodic, we are using the C++14 (ISO/IEC 14882:2014) standard.

Related to discussion on ros-planning/moveit#1146
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.