How to best handle itk::Math::Absolute

1

The recent PR 5802 has triggered a bunch of new warnings in RTK because “std::abs() returns the absolute value in the same type as the input, itk::Math::Absolute() returns the absolute value in the unsigned equivalent of the input type”. Looking closely at the warning

/home/srit/src/rtk/dashboard_tests/RTK/include/rtkJosephForwardProjectionImageFilter.hxx:178:1:   required from here
[CTest: warning matched] /home/srit/src/rtk/dashboard_tests/RTK/include/rtkJosephForwardProjectionImageFilter.hxx:383:28: warning: comparison of integer expressions of different signedness: 'int' and 'unsigned int' [-Wsign-compare]
  383 |         for (int i{ 0 }; i < itk::Math::Absolute(fs - ns) - 1; ++i)
      |                          ~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I think the behavior of the new itk::Absolute is undesirable in these cases and I am concerned this is not obvious to the user as the legacy itk::Math::abs calls itk::Math::Absolute. In this code, if fs equals ns, then you’d prefer itk::Absolute to return an int and not an unsigned int. So my questions are:

  • If you agree, should we keep the new itk::Absolute as is?
  • If yes, what is in your opinion the best option here? I can think of a static_cast of the value returned by Absolute or making i unsigned and moving the -1 to the left-hand side, but I find both much less readable than the original version… Any other suggestion?
2

Thanks for asking, @simon.rit

Another option in this specific case might be to use std::abs. :person_shrugging:

I think it’s a design choice for itk::Math::Absolute(x) to always return an unsigned integer type, for an integer argument. Hans (@hjmjohnson ), what do you think?

FYI, In the past itk::Math::abs(int x) already returned an unsigned int, going 10 years back in time:

ITK/Modules/Core/Common/include/itkMath.h at 70058bb6476cacc3350aeb5d08c60796b2de4483 · InsightSoftwareConsortium/ITK · GitHub

2 Likes
3

Good (and obvious) solution for std::abs which would have my preference now, thanks. I might as well drop the use of itk::Math::Absolute in my code.
I guess I’m concerned by the transparent change in behavior compared to one month ago if the signedness compilation warning is disabled (even if it was already like this 10 years ago).

1 Like