search

InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.
https://itk.org
Apache License 2.0
1.41k stars 663 forks source link

COMP: UnaryFunctorImageFilter RLEImage support on Apple Clang #4553

Closed thewtex closed 6 months ago

thewtex commented 6 months ago

With:

❯ c++ --version Apple clang version 15.0.0 (clang-1500.3.9.4) Target: arm64-apple-darwin23.3.0 Thread model: posix InstalledDir: /Library/Developer/CommandLineTools/usr/bin

Addresses:

/Users/matt.mccormick/src/RLETest/RLEIteratorTest.cxx:15:35: error: no viable constructor or deduction guide for deduction of template arguments of 'ImageScanlineConstIterator'
  itk::ImageScanlineConstIterator it2(constRLE, constRLE->GetLargestPossibleRegion());
                                  ^
/Users/matt.mccormick/src/ITK/Modules/Core/Common/include/itkImageScanlineConstIterator.h:106:3: note: candidate template ignored: substitution failure [with TImage = itk::RLEImage<short>]: cannot reference member of primary template because deduced class template specialization 'ImageScanlineConstIterator<itk::RLEImage<short, 3, unsigned short>>' is instantiated from a partial specialization
  ImageScanlineConstIterator(const TImage * ptr, const RegionType & region)
  ^                                                    ~~~~~~~~~~
/Users/matt.mccormick/src/ITK/Modules/Core/Common/include/itkImageScanlineConstIterator.h:267:1: note: candidate template ignored: could not match 'SmartPointer<TImage>' against 'const itk::RLEImage<short, 3> *'
ImageScanlineConstIterator(SmartPointer<TImage>, const typename TImage::RegionType &)
^
/Users/matt.mccormick/src/ITK/Modules/Core/Common/include/itkImageScanlineConstIterator.h:119:3: note: candidate function template not viable: requires single argument 'it', but 2 arguments were provided
  ImageScanlineConstIterator(const ImageIterator<TImage> & it)
  ^
/Users/matt.mccormick/src/ITK/Modules/Core/Common/include/itkImageScanlineConstIterator.h:135:3: note: candidate function template not viable: requires single argument 'it', but 2 arguments were provided
  ImageScanlineConstIterator(const ImageConstIterator<TImage> & it)
  ^
/Users/matt.mccormick/src/ITK/Modules/Core/Common/include/itkImageScanlineConstIterator.h:64:27: note: candidate function template not viable: requires 1 argument, but 2 were provided
class ITK_TEMPLATE_EXPORT ImageScanlineConstIterator : public ImageConstIterator<TImage>
                          ^
/Users/matt.mccormick/src/ITK/Modules/Core/Common/include/itkImageScanlineConstIterator.h:97:3: note: candidate function template not viable: requires 0 arguments, but 2 were provided
  ImageScanlineConstIterator()
  ^

Closes #4537.

N-Dekker commented 6 months ago

Thanks Matt, do I understand that this is just a quick-fix, to work around a compiler bug?

I still wonder:

thewtex commented 6 months ago

work around a compiler bug? possibly fixed in a newer clang version?

Well, this is for the compiler that comes with current XCode Clang (the default compiler on macOS), and we need to support it.

add "deduction guides" to the RLEImage specializations

Cool idea! Yes with a variation. Added here: https://github.com/KitwareMedical/ITKRLEImage/pull/63

dzenanz commented 6 months ago

Explicit template argument does not hurt, and it will allow compiling with older versions of RLEImage remote module. I vote to merge.

N-Dekker commented 6 months ago

Explicit template argument does not hurt, and it will allow compiling with older versions of RLEImage remote module.

It would hurt me! We put a lot of effort into using CTAD in ITK. And it was a pain already to work around those warnings (-wctad-maybe-unsupported). Is UnaryFunctorImageFilter then expected to be the only case where CTAD (temporarily?) may not be used?

N-Dekker commented 6 months ago

@thewtex If it is still found necessary to merge this PR, could you please add that four-letter-word (no pun intended 😸) "CTAD" somewhere in the subject of the commit? For example:

COMP: Revert CTAD in UnaryFunctorImageFilter to work around clang errors

It might help us in the future, when trying to find back this CTAD problem.

thewtex commented 6 months ago

Let's close in favor of https://github.com/KitwareMedical/ITKRLEImage/pull/63 to avoid additional warnings and bump the ITKRLEImage as @dzenanz suggested to improved compatibility #4555.