Disable uncrustify indentation check for macros that use windows __pragma

[emersonknapp]

Connects to ament/uncrustify_vendor#5

[clalancette]

Can you explain a bit more about what the new uncrustify doesn't like? I'm a bit confused as to why uncrustify doesn't like the below macros, but is happy with https://github.com/ros2/rcl/blob/master/rcl/src/rcl/logging_rosout.c#L45 , for instance.

[prajakta-gokhale]

I'm a bit confused as to why uncrustify doesn't like the below macros, but is happy with https://github.com/ros2/rcl/blob/master/rcl/src/rcl/logging_rosout.c#L45 , for instance.

I'm not yet sure why that file passes the test but this one doesn't. This is from the latest test run that @emersonknapp started without this fix, and it fails like so:

--- include/rcutils/stdatomic_helper/win32/stdatomic.h
35: +++ include/rcutils/stdatomic_helper/win32/stdatomic.h.uncrustify
35: @@ -205,19 +205,19 @@
35: -    switch (sizeof(out)) { \
35: -      case sizeof(uint64_t): \
35: -        out = _InterlockedCompareExchange64((LONGLONG *) object, desired, *expected); \
35: -        break; \
35: -      case sizeof(uint32_t): \
35: -        out = _InterlockedCompareExchange((LONG *) object, desired, *expected); \
35: -        break; \
35: -      case sizeof(uint16_t): \
35: -        out = _InterlockedCompareExchange16((SHORT *) object, desired, *expected); \
35: -        break; \
35: -      case sizeof(uint8_t): \
35: -        out = _InterlockedCompareExchange8((char *) object, desired, *expected); \
35: -        break; \
35: -      default: \
35: -        RCUTILS_LOG_ERROR_NAMED( \
35: -          _RCUTILS_PACKAGE_NAME, "Unsupported integer type in atomic_compare_exchange_strong"); \
35: -        exit(-1); \
35: -        break; \
35: -    } \
35: +  switch (sizeof(out)) { \
35: +  case sizeof(uint64_t): \
35: +  out = _InterlockedCompareExchange64((LONGLONG *) object, desired, *expected); \
35: +  break; \
35: +  case sizeof(uint32_t): \
35: +  out = _InterlockedCompareExchange((LONG *) object, desired, *expected); \
35: +  break; \
35: +  case sizeof(uint16_t): \
35: +  out = _InterlockedCompareExchange16((SHORT *) object, desired, *expected); \
35: +  break; \
35: +  case sizeof(uint8_t): \
35: +  out = _InterlockedCompareExchange8((char *) object, desired, *expected); \
35: +  break; \
35: +  default: \
35: +  RCUTILS_LOG_ERROR_NAMED( \
35: +  _RCUTILS_PACKAGE_NAME, "Unsupported integer type in atomic_compare_exchange_strong"); \
35: +  exit(-1); \
35: +  break; \
35: +  } \

...

1 files with code style divergence

...

10/11 Test #35: uncrustify .......................***Failed    1.55 sec

[emersonknapp]

@clalancette it seems to be the Windows __pragma operator that is throwing uncrustify off. When I remove them or replace with regular function calls, uncrustify does not fail.

This seems as if it's a regression from the older version, probably related to the addition of some explicit __pragma support a few months ago - uncrustify/uncrustify#2136 - see the final comment

I'm not entirely confident (but possibly just because I'm not familiar with this corner of the code) that this won't introduce corner cases

[emersonknapp]

I've opened this issue uncrustify/uncrustify#2314

[emersonknapp]

@clalancette do you have thoughts on what our next steps are for this? I perceive the following options

• Leave INDENT-OFF in this file, leaving a note that it's a known uncrustify issue for Windows macros
• Revert uncrustify_vendor to version it was before and cherry-pick the needed fix as a patch from Fix cast-align warnings in md5.cpp on armhf  uncrustify/uncrustify#2308

[emersonknapp]

@jacobperron do you know who could make a call on this? this is a prereq for the green armhf build

[jacobperron]

Possible third option: What if we try wrapping the definition of the macro in braces? Maybe it will satisfy uncrustify in the meantime? E.g.

{ \
  __pragma(warning(push)) \
  __pragma(warning(disable: 4244)) \
  __pragma(warning(disable: 4047)) \
  __pragma(warning(disable: 4024)) \
  do { \
    switch (sizeof(out)) { \
      case sizeof(uint64_t): \
        out = _InterlockedCompareExchange64((LONGLONG *) object, desired, *expected); \
        break; \
      case sizeof(uint32_t): \
        out = _InterlockedCompareExchange((LONG *) object, desired, *expected); \
        break; \
      case sizeof(uint16_t): \
        out = _InterlockedCompareExchange16((SHORT *) object, desired, *expected); \
        break; \
      case sizeof(uint8_t): \
        out = _InterlockedCompareExchange8((char *) object, desired, *expected); \
        break; \
      default: \
        RCUTILS_LOG_ERROR_NAMED( \
          _RCUTILS_PACKAGE_NAME, "Unsupported integer type in atomic_compare_exchange_strong"); \
        exit(-1); \
        break; \
    } \
  } while (0); \
  __pragma(warning(pop)) \
}

[jacobperron]

Otherwise, I'd also vote for option 1: leaving INDENT-OFF with a TODO to remove it once we switch to a version containing the patch.

[emersonknapp]

Unfortunately no dice with brace wrapping, everything after the __pragma still gets flattened.

That being the situation, I think we're left with the TODO note - I've updated it to reflect this.

[emersonknapp]

Running just a linux build with the updated uncrusify, without this change, as a sanity check to see what the complaint is, and that it's not just my dev environment.

[emersonknapp]

Running CI for just this change, since it should work fine with either uncrustify version and therefore could land first

• Linux
• Linux-aarch64
• macOS
• Windows

[jacobperron]

Okay with me. I'd like a second opinion though. @clalancette, thoughts?

[clalancette]

Sorry for being quiet here. I'm fine with disabling uncrustify here for now, until the upstream bug is fixed.