Avoid calling Windows max() and min() macros

[jacobperron]

Previously Windows CI was failing:
With this change:

[dirk-thomas]

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

[jacobperron]

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

I don't know why it started to happen, but I tried reverting ros/pluginlib#186 and the connected PRs (also reverting ros/class_loader#139) and the failures went away. Here's a job I ran with all of the changes reverted: https://ci.ros2.org/job/ci_windows/9917

[jacobperron]

Regardless of how it started happening, this seems like a good, defensive, patch to me. If users include rclcpp headers into their code that also happens to include Windows.h then they will see the same issue.

[wjwwood]

If windows.h is included before NOMINMAX is defined, it will not fix the issue. Most likely something we did in those prs changed that, either introduced windows.h earlier or we removed what was previously defining NOMINMAX, e.g. poco's headers.

[dirk-thomas]

I would suggest to try added the following before this newly added include https://github.com/ros2/rcutils/blob/f368c0f5b9666663f6c8e99b159221b4282d632c/include/rcutils/shared_library.h#L29:

#ifndef NOMINMAX
#define NOMINMAX
#endif

[wjwwood]

The parenthesis is still safer, because all someone has to do is this:

#include "windows.h"

// all of our headers

To break it again.

[wjwwood]

I'd recommend doing both, merging ros2/rcutils#229 and doing something to protect ourselves, I'd recommend this:

// This "dummy" macro is used to break up uses of min and max to avoid
// the min/max macros that may be defined by windows.h from breaking things.
// See: ...issue or additional resources here
#define PREVENT_WINDOWS_MIN_MAX_MACRO  // could be shorter, I was being explicit

// ...

  return (y > 0) && (x > (std::numeric_limits<T>::max PREVENT_WINDOWS_MIN_MAX_MACRO () - y));

// ...

Duration
Duration::max PREVENT_WINDOWS_MIN_MAX_MACRO  ()
{
  return Duration(std::numeric_limits<int32_t>::max PREVENT_WINDOWS_MIN_MAX_MACRO  (), 999999999);
}

This approach was suggested here:

https://stackoverflow.com/a/11550864

[jacobperron]

@wjwwood Why use a macro instead of parentheses?

[jacobperron]

Sadly, uncrustify doesn't like the parentheses (8dc3d6a).

[wjwwood]

@wjwwood Why use a macro instead of parentheses?

Because if I'm reading the code and see:

return Duration((std::numeric_limits<int32_t>::max)(), 999999999);

I think "that's weird, is that a mistake?" You could have a comment on each one, as you do in some places here, but if I see:

return Duration(std::numeric_limits<int32_t>::max PREVENT_WINDOWS_MIN_MAX_MACRO (), 999999999);

I think "What the heck is PREVENT_WINDOWS_MIN_MAX_MACRO?" and I go look it up, where I'll see some nice thorough comments about what it means.

But it's completely subjective.

[jacobperron]

I think #1054 is a better alternative, leaving code more readable.