COMP: Support non-EqualityComparable pixels Image template instantiation

[N-Dekker]

The ability to instantiate itk::Image<TPixel, VImageDimension> for a
non-EqualityComparable pixel type was broken by:

"ENH: Add operator== and operator!= to itk::Image"
pull request #2188
commit 7f3337d

Reported by Bradley Lowekamp:
issue #2583
"Explicit instantiation of itk::Image fails with 5.2 when pixel type
does not support operator=="

This commit fixes the issue by defining the operator== and operator!=
friend functions of itk::Image as function templates, rather than as
non-templates.

[Leengit]

Nice.

Do we want the template to include a NVImageDimension parameter too, and then check it for equality with VImageDimension ... so that image1==image2 will compile (and return false) if the two images have different image dimensions?

[N-Dekker]

Do we want the template to include a NVImageDimension parameter too, and then check it for equality with VImageDimension ... so that image1==image2 will compile (and return false) if the two images have different image dimensions?

Nice suggestion @Leengit I think I would rather have a compilation error on image1==image2, when their dimensions differ. But please let us know if you have a useful use case to support image1==image2, when their dimensions differ.

[dzenanz]

If it compiles and works as intended, good.

[N-Dekker]

If it compiles and works as intended, good.

I must say, that Linux-warning is non-ObVious 😸 to me: https://open.cdash.org/viewBuildError.php?type=1&buildid=7271820

Modules/Core/Common/include/itkImage.hxx:79:1: warning: 'void itk::Image<TPixel, VImageDimension>::FillBuffer(const TPixel&) [with TPixel = {anonymous}::NonEqualityComparableType; unsigned int VImageDimension = 2]' defined but not used [-Wunused-function]

Any suggestion?

[dzenanz]

Any suggestion?

Sadly, no. It is just as cryptic to me.

[N-Dekker]

@dzenanz Just fixed the Linux GCC warning, which said:

'FillBuffer(const TPixel&)' defined but not used [-Wunused-function]

The fix (as I just force-pushed) was to simply use the function, en another unit test! 😄

[Leengit]

In itkImage.h, is this line correct?:

const TPixel * const lhsBufferPointer = lhsBuffer.GetBufferPointer();

I worry that TPixel is the pixel type of the right-hand side, but not necessarily of the left-hand side.

[N-Dekker]

In itkImage.h, is this line correct?:

const TPixel * const lhsBufferPointer = lhsBuffer.GetBufferPointer();

I worry that TPixel is the pixel type of the right-hand side, but not necessarily of the left-hand side.

Well observed @Leengit Would you like it better if it would use auto instead?

Please keep in mind that the aim of this pull request is just to allow itk::Image template instantiations for non-EqualityComparable pixel types. It isn't meant to further enhance operator==.

[Leengit]

In itkImage.h, is this line correct?:

const TPixel * const lhsBufferPointer = lhsBuffer.GetBufferPointer();

I worry that TPixel is the pixel type of the right-hand side, but not necessarily of the left-hand side.

Well observed @Leengit Would you like it better if it would use auto instead?

Please keep in mind that the aim of this pull request is just to allow itk::Image template instantiations for non-EqualityComparable pixel types. It isn't meant to further enhance operator==.

So that the compiler catches type errors for me, I usually use the actual type if I can determine it and if it isn't completely obvious -- e.g., because we just cast to it on the right-hand side. In this case, I believe that that would be TEqualityComparable, but auto would work too.

[N-Dekker]

So that the compiler catches type errors for me, I usually use the actual type if I can determine it and if it isn't completely obvious -- e.g., because we just cast to it on the right-hand side. In this case, I believe that that would be TEqualityComparable, but auto would work too.

OK @Leengit If I amend the commit to use TEqualityComparable, will you approve the pull request for me? 😸

[Leengit]

So that the compiler catches type errors for me, I usually use the actual type if I can determine it and if it isn't completely obvious -- e.g., because we just cast to it on the right-hand side. In this case, I believe that that would be TEqualityComparable, but auto would work too.

OK @Leengit If I amend the commit to use TEqualityComparable, will you approve the pull request for me? smile_cat

It's a deal.

[N-Dekker]

OK @Leengit If I amend the commit to use TEqualityComparable, will you approve the pull request for me? smile_cat

It's a deal.

@Leengit Thanks! Please check the amend + force-push: https://github.com/InsightSoftwareConsortium/ITK/compare/4549ea34b6f124cce48c6dcf7ba33aea9476e4b9..b02c9cfdb9788cbd02388f3579e26903a1654f52 Hope you appreciate the static_assert that I added with the amend. 😃

[Leengit]

@Leengit Thanks! Please check the amend + force-push: https://github.com/InsightSoftwareConsortium/ITK/compare/4549ea34b6f124cce48c6dcf7ba33aea9476e4b9..b02c9cfdb9788cbd02388f3579e26903a1654f52 Hope you appreciate the static_assert that I added with the amend. smiley

The TEqualityComparable for lhsBufferPointer is good, thank you.

Is the thinking that the static_assert will make sure that operator== is only ever used the way it was before this pull request? That seems reasonable until we have a need for and an implementation of a more general use of operator== for images.

[N-Dekker]

Is the thinking that the static_assert will make sure that operator== is only ever used the way it was before this pull request?

Indeed 😃

That seems reasonable until we have a need for and an implementation of a more general use of operator== for images.

Yes, exactly. The static_assert may be removed once (if ever) we need operator== to compare two images of different types. And then of course some extra unit tests must be added. But for now, at least the template instantiation issue should be fixed.

[N-Dekker]

Update: force-pushed, now using C++ SFINAE (std::enable_if) (instead of a static_assert) to ensure that the images passed to operator== and operator!= have the correct pixel type TPixel.

[blowekamp]

Thanks for addressing the reported issue 👍