conditionally undefine NO_ERROR for Windows.

[seanyen]

In ROS1, this treatment is added to gencpp to reduce the odds of name collision when the downstream project includes Windows headers. And I am adding the same treatment for ROS2 here.

[seanyen]

@jacobperron @dirk-thomas This is ready for review and merge. Thanks.

[dirk-thomas]

I would ask the same questions as in ros2/rclcpp#1053 (comment):

Do we have any idea why this started to happen? I think that would be essential to decide what the proper fix is.

[cottsay]

• Linux
• Linux-aarch64
• macOS
• Windows

[tfoote]

I suspect that this is failing now due to a header include ordering change which is a side effect of one of the other refactors. Something such as as @ahcorde 's changes to pluginlib's implementation would be strong candidates for changing the ordering: ros/pluginlib#186 to end up with windows.h evalutated before this.

[jacobperron]

I agree, when I reverted ros/pluginlib#186 (and related PRs) the failures in rclcpp disappeared. I suspect the same would be true for the this case too.

[jacobperron]

The answer is ros/pluginlib#186 introduced the include rcpputils/shared_library.hpp, which includes rcutils/shared_library.h which includes windows.h

[jacobperron]

Also, CI never passed originally 🙁 ros/pluginlib#186 (comment)

[dirk-thomas]

CI never passed originally 🙁 ros/pluginlib#186 (comment)

@ahcorde FYI

[dirk-thomas]

I propose to move the #include <windows.h> from the header in to the .cpp files and replace the type in the header with either a forward declaration or a void *.

[cottsay]

I propose to move the #include <windows.h> from the header in to the .cpp files and replace the type in the header with either a forward declaration or a void *.

I'll look into this.

[tfoote]

We can make sure not to trigger this ourselves by shuffling the includes in pluginlib. However we have no control of how the user uses this and they may explicitly add `#include <windows.h> before including this header directly which will cause the same problem. I think we should land this independent of the changes in pluginlib.

[cottsay]

However we have no control of how the user uses this and they may explicitly add #include <windows.h> before including this header...

I don't see that problem as unique to Windows.h. If someone includes one of our headers and another header with colliding definitions, I'm not sure that's something we should be trying to support.

[lilustga]

This same issue of macro collision cropped up while building moveit_ros_planning_interface for moveit2 on Windows.

The error is:
C:\opt\ros\eloquent\x64\include\tf2/exceptions.h(43,3): error C2059: syntax error: 'constant'

[clalancette]

@seanyen I think the right way to fix this is as follows:

1. Modify the enum class TF2Error below so that it looks something like:

enum class TF2Error : std::uint8_t
{
  TF2_NO_ERROR = 0,
  TF2_LOOKUP_ERROR = 1,
  TF2_CONNECTIVITY_ERROR = 2,
  TF2_EXTRAPOLATION_ERROR = 3,
  TF2_INVALID_ARGUMENT_ERROR = 4,
  TF2_TIMEOUT_ERROR = 5,
  TF2_TRANSFORM_ERROR = 6,

  NO_ERROR [[deprecated]] = 0,
  LOOKUP_ERROR [[deprecated]] = 1,
  CONNECTIVITY_ERROR [[deprecated]] = 2,
  EXTRAPOLATION_ERROR [[deprecated]] = 3,
  INVALID_ARGUMENT_ERROR [[deprecated]] = 4,
  TIMEOUT_ERROR [[deprecated]] = 5,
  TRANSFORM_ERROR [[deprecated]] = 6
};

(my understanding is that the [[deprecated]] syntax is standard in C++14, so should work with MSVC as well, but please let me know if that is not the case)

2. Fix the callers in the tree that use the deprecated enum values.
3. Still put the workaround above in for Galactic.
4. After Galactic, remove the deprecated enum values.

Does that make sense?

[seanyen]

This proposal is totally making sense to me. I like the idea to prefix the enum to avoid the collision entirely. [[deprecated]] is supported by the latest MSVC, so we should be good too. Do you want me to rework this pull request or you already have something on the way?

[clalancette]

Do you want me to rework this pull request or you already have something on the way?

If you could rework this PR, that would be great. Keep in mind you'll probably have to change the users of the enum in other packages within geometry2 (it looks like at least tf2_ros, tf2_py, and tf2_ros_py).

Thanks!

[clalancette]

See #349, which implements this (I needed it for other reasons). I'm going to go ahead and close this PR out in favor of that one.