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

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

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.?

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

Kinetic is C++11

Melodic is C++14

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

@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

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.

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...

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: moveit/moveit_robots#71

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

@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.

@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

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

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".

@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: 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)).

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.

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.

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.

@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.

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.

@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 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

@davetcoleman wrote:

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

ros/ros_comm#1525 has been merged.

ros/ros_comm#1525 has been merged.

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

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.

@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.

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

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 :)

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

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.

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

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.

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?