ENH: Add C++17 [[nodiscard]] to Image "Transform" member functions

[N-Dekker]

It would usually be a mistake when a user would unintentionally ignore the return value of those member functions. Specifically, the overloads of TransformPhysicalPointToIndex and TransformPhysicalPointToContinuousIndex that have two parameters both return a bool value. If this return value can be ignored, it is preferable (for performance reasons) to call the corresponding overload that has one parameter instead.

The compiler may now produce a warning when the return value of any of those member functions is unintentionally ignored.

[dzenanz]

We should add "If this return value can be
ignored, it is preferable (for performance reasons) to call the corresponding
overload that has one parameter instead." to the documentation of these methods, so if someone does get a warning they can easily figure out what to do.

[N-Dekker]

We should add "If this return value can be ignored, it is preferable (for performance reasons) to call the corresponding
overload that has one parameter instead." to the documentation of these methods, so if someone does get a warning they can easily figure out what to do.

Thanks @dzenanz I agree, will add something like that with a force-push to this PR.

Note that C++20 allows adding a message to the nodiscard attribute:

 [[nodiscard("If this return value can be ignored, please call the overload that has one parameter!")]]

When does ITK upgrade to C++20? 😸

[dzenanz]

Maybe also add a new macro, called ITK_NO_DISCARD(message), which will simplify to [[nodiscard("message")]] for C++20, and [[nodiscard]] for C++17?

[N-Dekker]

Maybe also add a new macro, called ITK_NO_DISCARD(message), which will simplify to [[nodiscard("message")]] for C++20, and [[nodiscard]] for C++17?

@dzenanz Can you please make that a pull request? 😃

[dzenanz]

You missed a couple in Examples/DataRepresentation/Path/PolyLineParametricPath1.cxx:105:3: warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]

[dzenanz]

Quite a fallout from this PR, 27 new warnings.

[dzenanz]

And if some of the remote modules are included, the count goes up to 72:
https://open.cdash.org/viewBuildError.php?type=1&buildid=8601550

[N-Dekker]

And if some of the remote modules are included, the count goes up to 72:
https://open.cdash.org/viewBuildError.php?type=1&buildid=8601550

@dzenanz This means that those [[nodiscard]] attributes are really helpful. On the other hand, they may be annoying to some users. Would it be helpful to place the attributes inside an #ifdef ITK_LEGACY_REMOVE?

[dzenanz]

It would probably be helpful to put it under legacy ifdef. That would be easier to accomplish if we go the macro route: #3993 (comment) 😄

[hjmjohnson]

@N-Dekker I think it would be best to put it inside an ITK_LEGACY_REMOVE section.

Better yet to to combine @dzenanz previous suggestions

#ifdef ITK_LEGACY_REMOVE
#. if C++ > 20
#    define  ITK_NO_DISCARD(message)  [[nodiscard("message")]] 
# else 
#  define  ITK_NO_DISCARD(message)  [[nodiscard]] 
# endif
#else

#  define ITK_NO_DISCARD(message)
#endif

[N-Dekker]

Sorry I don't really like the idea of making [[nodiscard]] a legacy-remove-only feature forever. If that is what you mean. Suppose a brand new function is added, GetImportantValue(). This function might be marked [[nodiscard]] either with or without ITK_LEGACY_REMOVE.

But maybe I just don't understand your proposal.

[dzenanz]

I don't think that anything is a legacy-remove-only feature forever. During major version upgrades, we systematically remove legacy behaviors, only leaving the new behaviors. But for now, only people with ITK_LEGACY_REMOVE on should get this warning.

[N-Dekker]

In practice, these warnings only appear in legacy code, calling the old TransformPhysicalPointToIndex or the old TransformPhysicalPointToContinuousIndex (both returning a bool), right ? They intend to inform the user that they should either use the return value (telling them whether the point is inside or outside), or use the other overload.

The other uses of [[nodiscard]] added by this PR do not cause any problem, right? So the other uses would not need to be replaced by macro's, right?

[dzenanz]

The other uses of [[nodiscard]] added by this PR

Which others?

[N-Dekker]

The other uses of [[nodiscard]] added by this PR
Which others?

This PR added [[nodiscard]] to 8 ImageBase member functions. I believe that only two of them actually trigger warnings in existing (legacy) code. The other 6 are harmless. Please take another look at da2af56

[dzenanz]

Ah, I now understand what you meant. It is wholly pointless to call those transform methods if you don't use the result. So yes, in those cases [[nodiscard]] can be directly applied, bypassing legacy.

[N-Dekker]

Thanks, so then we could decide to use plain [[nodiscard]] for new functionality, and LEGACY_REMOVE_NODISCARD for old code. Just thinking aloud 🤔

[hjmjohnson]

I disagree with @dzenanz a little bit. It may be pointless in context of ITK as it exists today, but using the LEGACY_REMOVE approach allows a smooth transition to enforcing the new preferred mechanism over time. It takes a long time to allow transitioning old code to new interfaces overtime. There still exists a lot of code that depends on ITK that was written before the new non-bounds checked version of the "Point Transform*To*(const Point)" functions existed.

[N-Dekker]

There still exists a lot of code that depends on ITK that was written before the new non-bounds checked version of the "Point TransformTo(const Point)" functions existed.

Just some additional info here: the extra overloads for ImageBase Transform member functions were added with pull request #868 commit 65233e3, almost four years ago, and they were included already with the very first release of ITK 5.0 Please note also that this PR does not propose to deprecate the old versions. It only just warns the user who calls the old TransformPhysicalPointToIndex or TransformPhysicalPointToContinuousIndex when the return value is unused. I think such a warning is appropriate. Because users should be aware of those IsInside calls for each point, when using the old TransformPhysicalPointToIndex or TransformPhysicalPointToContinuousIndex.

[hjmjohnson]

@N-Dekker I don't disagree, but some in the community will. The macros were suggested as a path of least community frustration, along with the benefit of allowing really nice messages for C++20 users. Many in the community only want to use tools built on ITK, and don't want the burden of digging through warnings. Those of us who do the development want the ability to see those warnings, but we often do not have time to change all depenadant poorly written code all at once.

When working on data analysis, I like to build ITK_LEGACY_REMOVE=OFF so that I can focus on problems that may be related to the data analysis. When writing code or testing performance, I use ITK_LEGACY_REMOVE=ON to prepare for the future.

This comment is to present my POV. I am OK with either solution. Personally I am not affect much either way.

[issakomi]

IMHO, TransformPhysicalPointToContinuousIndex is not ideal candidate for [[nodiscard]], honestly.
This PR affects my code too. I think it is OK to discard a return value of the TransformPhysicalPointToContinuousIndex sometimes. I use the function to draw intersection lines and it doesn't matter is the point inside or outside, i just need continuous index.

A slice of one image is represented as infinite collision plane and 4 rays are send from the edges of the slice of another image, so inside or outside is not important at this point. (It is done this way to work with non-uniform DICOM series, every slice is defined in physical space).

Here is small trivial test to show what happens if the point is outside.

#include <itkImage.h>
#include <iostream>
int main(int, char**)
{
  using ImageType = itk::Image<unsigned char, 2>;
  ImageType::IndexType index{};
  ImageType::SizeType size{ { 128, 128 } };
  ImageType::RegionType region;
  region.SetIndex(index);
  region.SetSize(size);
  ImageType::PointType origin{};
  ImageType::SpacingType spacing{ { 0.5, 0.5 } };
  ImageType::Pointer image = ImageType::New();
  image->SetRegions(region);
  image->SetOrigin(origin);
  image->SetSpacing(spacing);
  image->Allocate();
  ImageType::PointType point{ { -1.0, -1.0 } };
  itk::ContinuousIndex<float, 2> cindex;
  const bool ok = image->TransformPhysicalPointToContinuousIndex(point, cindex);
  std::cout << (ok ? "inside, " : "outsize, ") << cindex[0] << " " << cindex[1] << std::endl;
  return 0;
}

r@deb2:~/tmp/test7/build$ ./test 
outsize, -2 -2

cc @dzenanz

Edit: OK, I have already fixed the warning with const bool is_inside = (void)is_inside.

[N-Dekker]

I think it is OK to discard a return value of the TransformPhysicalPointToContinuousIndex sometimes. I use the function to draw intersection lines and it doesn't matter is the point inside or outside, i just need continuous index.

Thanks for your extensive comment @issakomi Indeed the warning can be suppressed quite easily in ITK user code, either by a (void) cast of the return value or by assigning the result to C++17 std::ignore (as preferred by http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es48-avoid-casts). However, if you do not need the bool return value of TransformPhysicalPointToContinuousIndex(point, index), it is preferable to use the overload that has just one parameter, and returns the index. Because then the estimation if the point is inside is entirely avoided.

[issakomi]

it is preferable to use the overload that has just one parameter, and returns the index

I am not sure that the overload was in ITK 4. I think Fedora still has no ITK 5 devel. package. I am not sure, I don't maintain the Fedora package myself. Probably I shall use preprocessor to support both ways depending on ITK version. Flathub distribution is easier, I can link statically with the latest ITK.

#if ((ITK_VERSION_MAJOR == 5 && ITK_VERSION_MINOR >= 4) || ITK_VERSION_MAJOR > 5)
              const auto index =
                image->TransformPhysicalPointToContinuousIndex<float>(point);
#else
              itk::ContinuousIndex<float, 3> index;
              image->TransformPhysicalPointToContinuousIndex(point,index);
#endif

[issakomi]

OK, have to admit there is in fact an improvement if inside/outside things are skipped.

[N-Dekker]

OK, have to admit there is in fact an improvement if inside/outside things are skipped.

Thanks @issakomi. On the other side, I have to admit that the calling the TransformPhysicalPointToContinuousIndex<TIndexRep, TCoordRep> overload may be inconvenient, because as a user, you'd have to explicitly specify the index value type (TIndexRep). And because you'd sometimes have to add a template keyword (PR #4005). I'm hoping that extra non-templated overloads may be of help:

• PR (Not ready) Simpler TransformPhysicalPointToContinuousIndex(point) overloads #4002

By the way, would it help you if this specific [[nodiscard]] would be made "ITK_LEGACY_REMOVE only"? (Meaning that those warnings would not appear in a "legacy support build".)

[dzenanz]

If an application wants to support both ITK 5.4+, and ITK5.3-, an #ifdef as @issakomi suggests will be an appealing option. So I still think that the best approach is via the macro (#3993 (comment)).

[dzenanz]

I turned the proposed changes into a PR: #4007.