moveit / geometric_shapes

Representation of geometric shapes
57 stars 92 forks source link

Foxy support via __has_include #214

Closed tylerjw closed 2 years ago

tylerjw commented 2 years ago

__has_include is a C++17 feature (Reference). We can support foxy from the ros2 branch with this change. The downside to this is foxy doesn't officially support C++17. This is the reason I originally created the foxy branch. If we are ok using C++17 on Foxy we can support all three current ros2 versions (Foxy, Galactic, Rolling) from one branch. This will also fix the semvar issues created by lot leaving room for more foxy versions (as we can just release foxy from the ros2 branch then).

codecov[bot] commented 2 years ago

Codecov Report

Merging #214 (8618b2e) into ros2 (a8c60ec) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             ros2     #214   +/-   ##
=======================================
  Coverage   54.66%   54.66%           
=======================================
  Files          11       11           
  Lines        2031     2031           
=======================================
  Hits         1110     1110           
  Misses        921      921           
Impacted Files Coverage Δ
src/mesh_operations.cpp 44.55% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a8c60ec...8618b2e. Read the comment docs.

tylerjw commented 2 years ago

Alternatively, to avoid C++17, you could test for the existence of one or the other header file in cmake and add a corresponding define.

Do you have an example for how to do that?

tylerjw commented 2 years ago

I'm biased in that I think it is really silly that Foxy is using C++14 when the two Tier 1 OS's have C++17 by default in their compilers (Ubuntu 20.04 and Windows). In my opinion, the version of C++ that ROS uses should just follow the defaults in the compiler for whatever the first-class targets are.

tylerjw commented 2 years ago

The natural objection is something along the lines of "but some embedded target might not have a new enough compiler". I would challenge those who are saying that to go find those compilers and make a list of them somewhere publically. In my experience, every embedded target I've used had compilers that support newer versions of C++ about as fast as the ones on linux. I challenge anyone to find a microcontroller (in the currently supported version of the microcontroller from the manufacturer) that you cannot find a compiler with C++20 support. It is hard enough to get people to upgrade ROS who are on linux... I can't imagine there are enough people using an EOL micro-controller wanting to upgrade their ROS version but not their micro-controller and compiler.

tylerjw commented 2 years ago

Sorry for the off-topic rant. I'm ok with a define from cmake if you have an example of how to search for a header in cmake and set the variable.

v4hn commented 2 years ago

Example. That was the (rather verbose) way to go for many years. :-)

I believe @rhaschke just meant to point out an alternative. Personally I don't see a problem with using __has_include. It is a very neat feature. If, against all odds, someone should complain about it, it's straight-forward to patch it out.

tylerjw commented 2 years ago

Example. That was the (rather verbose) way to go for many years. :-)

I believe @rhaschke just meant to point out an alternative. Personally I don't see a problem with using __has_include. It is a very neat feature. If, against all odds, someone should complain about it, it's straight-forward to patch it out.

I like the solution here better. If either of you cares about this strongly I'm happy to change it though. I am just happy we can fix the SemVar problem I created and get rid of that foxy branch.