SuperElastix / elastix

Official elastix repository
http://elastix.dev
Apache License 2.0
474 stars 116 forks source link

COMP: Remove ITK_DISALLOW_COPY_AND_MOVE from UserData struct's #1202

Closed thewtex closed 1 month ago

thewtex commented 2 months ago

On GCC 12.2.2 (Debian):

/home/matt/src/elastix/Common/ImageSamplers/itkImageGridSampler.hxx: In instantiation of ‘static void itk::ImageGridSampler::MultiThreadedGenerateData(itk::MultiThreaderBase&, itk::ThreadIdType, const TInputImage&, const MaskType*, const typename Superclass::InputImageRegionType&, const SampleGridSpacingType&, std::vector<typename itk::ImageSamplerBase::ImageSampleType>&) [with TInputImage = itk::Image<short int, 4>; itk::ThreadIdType = unsigned int; MaskType = itk::ImageMaskSpatialObject<4, unsigned char>; typename Superclass::InputImageRegionType = itk::ImageRegion<4>; Superclass = itk::ImageSamplerBase<itk::Image<short int, 4> >; SampleGridSpacingType = itk::Offset<4>; typename itk::ImageSamplerBase::ImageSampleType = itk::ImageSample<itk::Image<short int, 4> >]’: /home/matt/src/elastix/Common/ImageSamplers/itkImageGridSampler.hxx:277:30: required from ‘void itk::ImageGridSampler::GenerateData() [with TInputImage = itk::Image<short int, 4>]’ /home/matt/src/elastix/Common/ImageSamplers/itkImageGridSampler.hxx:253:1: required from here /home/matt/src/elastix/Common/ImageSamplers/itkImageGridSampler.hxx:212:12: error: no matching function for call to ‘itk::ImageGridSampler<itk::Image<short int, 4> >::UserData::UserData()’ 212 | UserData userData{ inputImage, | ^~~~ /home/matt/src/elastix/Common/ImageSamplers/itkImageGridSampler.h:169:32: note: candidate: ‘itk::ImageGridSampler::UserData::UserData(itk::ImageGridSampler::UserData&&) [with TInputImage = itk::Image<short int, 4>]’ (deleted) 169 | ITK_DISALLOW_COPY_AND_MOVE(UserData); | ^~~~ /home/matt/src/ITK/Modules/Core/Common/include/itkMacro.h:397:3: note: in definition of macro ‘ITK_DISALLOW_COPY_AND_MOVE’ 397 | TypeName(TypeName &&) = delete; \ | ^~~~ /home/matt/src/elastix/Common/ImageSamplers/itkImageGridSampler.h:169:32: note: candidate expects 1 argument, 4 provided 169 | ITK_DISALLOW_COPY_AND_MOVE(UserData); | ^~~~ /home/matt/src/ITK/Modules/Core/Common/include/itkMacro.h:397:3: note: in definition of macro ‘ITK_DISALLOW_COPY_AND_MOVE’ 397 | TypeName(TypeName &&) = delete; \ | ^~~~ /home/matt/src/elastix/Common/ImageSamplers/itkImageGridSampler.h:169:32: note: candidate: ‘itk::ImageGridSampler::UserData::UserData(const itk::ImageGridSampler::UserData&) [with TInputImage = itk::Image<short int, 4>]’ (deleted) 169 | ITK_DISALLOW_COPY_AND_MOVE(UserData); | ^~~~ /home/matt/src/ITK/Modules/Core/Common/include/itkMacro.h:395:3: note: in definition of macro ‘ITK_DISALLOW_COPY_AND_MOVE’ 395 | TypeName(const TypeName &) = delete; \ | ^~~~ /home/matt/src/elastix/Common/ImageSamplers/itkImageGridSampler.h:169:32: note: candidate expects 1 argument, 4 provided 169 | ITK_DISALLOW_COPY_AND_MOVE(UserData); | ^~~~ /home/matt/src/ITK/Modules/Core/Common/include/itkMacro.h:395:3: note: in definition of macro ‘ITK_DISALLOW_COPY_AND_MOVE’ 395 | TypeName(const TypeName &) = delete; \ | ^~~~

thewtex commented 2 months ago

@N-Dekker there may be a better solution to keep some of the constraints, but this was a quick fix to get the build to succeed on my system.

N-Dekker commented 2 months ago

Thanks @thewtex Do you have a link to the entire log? I'm still trying to reproduce the error, but I haven't seen it yet:

The following just compiles: https://godbolt.org/z/3jejxcqMa

#include <vector>

#define ITK_DISALLOW_COPY_AND_MOVE(TypeName)       \
  TypeName(const TypeName &) = delete;             \
  TypeName & operator=(const TypeName &) = delete; \
  TypeName(TypeName &&) = delete;                  \
  TypeName & operator=(TypeName &&) = delete

using InputImageType = int;
using MaskType = int;
using SampleGridSpacingType = int;
using WorkUnit = int;

  struct UserData
  {
    ITK_DISALLOW_COPY_AND_MOVE(UserData);

    const InputImageType &      InputImage;
    const MaskType * const      Mask{};
    const SampleGridSpacingType GridSpacing{};
    std::vector<WorkUnit>       WorkUnits{};
  };

std::vector<WorkUnit> GenerateWorkUnits();

void f(
    const InputImageType &      inputImage,
    const MaskType * const      mask,
    const SampleGridSpacingType gridSpacing
)
{
  UserData userData{ inputImage,
                     mask,
                     gridSpacing,
                      GenerateWorkUnits() };
}
thewtex commented 2 months ago

@N-Dekker the commit / pr comment has the full error. There are many more emitted for all the locations where the macro was present.

N-Dekker commented 2 months ago

There are many more emitted for all the locations where the macro was present.

Thanks Matt. So then, should the errors be reproducible by the code I just posted? At https://github.com/SuperElastix/elastix/pull/1202#issuecomment-2238940875 and https://godbolt.org/z/3jejxcqMa ? It looks like a compiler bug, but I'm not sure 🤷

Such ITK_DISALLOW_COPY_AND_MOVE(UserData) calls have been in there for six months already (for example, pull request #1017), so I'm surprised that these errors did not occur before. And I wonder if there's anything wrong about the code. Or is it just a compiler bug?


@thewtex Can you please tell me, did these errors occur only at a local build, or also at a CI build? Is the full compilation log available online? I'm especially interested to see the compiler command-line flags.

Would it be possible for you to reproduce the errors by just compiling the minimal example from https://godbolt.org/z/3jejxcqMa ?

I think it's important to know if there is indeed a limitation to the use of ITK_DISALLOW_COPY_AND_MOVE.

The errors do appear at https://godbolt.org/z/EvTcnPx13 when doing a C++11 build, using GCC compiler flag -std=c++11. Which is why I would like to see the compiler flags on your GCC 12.2.2 build. 🤷

N-Dekker commented 1 month ago

@thewtex Finally I managed to reproduce your issue! At https://github.com/InsightSoftwareConsortium/ITKElastix/actions/runs/10575617835/job/29299639880


/emsdk/upstream/emscripten/em++ -DITK_FFTIMAGEFILTERINIT_FACTORY_REGISTER_MANAGER -I/work/emscripten-build/_deps/elx-build/ITKFactoryRegistration -I/ITK/Modules/IO/TransformFactory/include -I/ITK/Modules/IO/ImageBase/include -I/ITK-build/Modules/IO/ImageBase -I/ITK-build/Modules/ThirdParty/DoubleConversion/src/double-conversion -I/ITK/Modules/ThirdParty/DoubleConversion/src -I/ITK/Modules/Core/TestKernel/include -I/ITK/Modules/Filtering/ImageFeature/include -I/ITK/Modules/Registration/Common/include -I/ITK/Modules/Numerics/Optimizers/include -I/ITK/Modules/Core/Mesh/include -I/ITK/Modules/Filtering/ImageGradient/include -I/ITK/Modules/ThirdParty/Expat/src/expat -I/ITK-build/Modules/ThirdParty/Expat/src/expat -I/ITK/Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition -I/ITK/Modules/ThirdParty/GDCM/src/gdcm/Source/MessageExchangeDefinition -I/ITK/Modules/ThirdParty/GDCM/src/gdcm/Source/InformationObjectDefinition -I/ITK/Modules/ThirdParty/GDCM/src/gdcm/Source/Common -I/ITK/Modules/ThirdParty/GDCM/src/gdcm/Source/DataDictionary -I/ITK/Modules/ThirdParty/GDCM/src/gdcm/Source/MediaStorageAndFileFormat -I/ITK-build/Modules/ThirdParty/GDCM/src/gdcm/Source/Common -I/ITK-build/Modules/ThirdParty/GDCM -I/ITK/Modules/Core/FiniteDifference/include -I/ITK/Modules/Filtering/CurvatureFlow/include -I/ITK/Modules/Numerics/NarrowBand/include -I/ITK/Modules/Segmentation/ConnectedComponents/include -I/ITK/Modules/Filtering/MathematicalMorphology/include -I/ITK/Modules/Filtering/ImageLabel/include -I/ITK/Modules/Filtering/LabelMap/include -I/ITK/Modules/Filtering/BinaryMathematicalMorphology/include -I/ITK/Modules/Filtering/DistanceMap/include -I/ITK/Modules/Filtering/ImageSources/include -I/ITK/Modules/Filtering/Thresholding/include -I/ITK/Modules/Filtering/FFT/include -I/ITK/Modules/Filtering/Convolution/include -I/ITK/Modules/Filtering/Smoothing/include -I/ITK/Modules/Filtering/Path/include -I/ITK/Modules/ThirdParty/ZLIB/src -I/ITK-build/Modules/ThirdParty/ZLIB/src -I/ITK/Modules/ThirdParty/MetaIO/src/MetaIO/src -I/ITK-build/Modules/ThirdParty/MetaIO/src/MetaIO/src -I/ITK/Modules/Core/SpatialObjects/include -I/ITK/Modules/Filtering/ImageCompose/include -I/ITK/Modules/Filtering/ImageStatistics/include -I/ITK/Modules/Filtering/ImageIntensity/include -I/ITK/Modules/Filtering/ImageFilterBase/include -I/ITK/Modules/Core/Transform/include -I/ITK-build/Modules/ThirdParty/Netlib -I/ITK/Modules/Numerics/Statistics/include -I/ITK/Modules/Core/ImageAdaptors/include -I/ITK/Modules/Core/ImageFunction/include -I/ITK/Modules/Filtering/ImageGrid/include -I/ITK/Modules/Filtering/DisplacementField/include -I/ITK-build/Modules/ThirdParty/VNL/src/vxl/core -I/ITK-build/Modules/ThirdParty/VNL/src/vxl/vcl -I/ITK-build/Modules/ThirdParty/VNL/src/vxl/v3p/netlib -I/ITK/Modules/ThirdParty/VNL/src/vxl/core -I/ITK-build/Modules/ThirdParty/Eigen3/src -I/ITK/Modules/Core/Common/include -I/ITK-build/Modules/Core/Common -I/work/emscripten-build/_deps/elx-src/Common -I/work/emscripten-build/_deps/elx-src/Common/CostFunctions -I/work/emscripten-build/_deps/elx-src/Common/ImageSamplers -I/work/emscripten-build/_deps/elx-src/Common/LineSearchOptimizers -I/work/emscripten-build/_deps/elx-src/Common/ParameterFileParser -I/work/emscripten-build/_deps/elx-src/Common/Transforms -I/work/emscripten-build/_deps/elx-src/Common/MevisDicomTiff -I/work/emscripten-build/_deps/elx-src/Core -I/work/emscripten-build/_deps/elx-src/Core/Install -I/work/emscripten-build/_deps/elx-src/Core/Kernel -I/work/emscripten-build/_deps/elx-src/Core/ComponentBaseClasses -I/work/emscripten-build/_deps/elx-src/Core/Configuration -I/work/emscripten-build/_deps/elx-src/Core/Main -I/work/emscripten-build/_deps/elx-src/Components/FixedImagePyramids -I/work/emscripten-build/_deps/elx-src/Components/ImageSamplers -I/work/emscripten-build/_deps/elx-src/Components/Interpolators -I/work/emscripten-build/_deps/elx-src/Components/Metrics -I/work/emscripten-build/_deps/elx-src/Components/MovingImagePyramids -I/work/emscripten-build/_deps/elx-src/Components/Optimizers -I/work/emscripten-build/_deps/elx-src/Components/Registrations -I/work/emscripten-build/_deps/elx-src/Components/ResampleInterpolators -I/work/emscripten-build/_deps/elx-src/Components/Resamplers -I/work/emscripten-build/_deps/elx-src/Components/Transforms -I/work/emscripten-build/_deps/elx-build -isystem /ITK-build/Modules/ThirdParty/ZLIB/src/itkzlib-ng -isystem /ITK/Modules/ThirdParty/VNL/src/vxl/vcl -isystem /ITK/Modules/ThirdParty/VNL/src/vxl/v3p/netlib -isystem /ITK-build/Modules/ThirdParty/KWSys/src -isystem /ITK/Modules/ThirdParty/Eigen3/src/itkeigen/.. -isystem /ITK/Modules/ThirdParty/VNL/src/vxl/core/vnl/algo -isystem /ITK/Modules/ThirdParty/VNL/src/vxl/core/vnl -isystem /ITK-build/Modules/ThirdParty/VNL/src/vxl/core/vnl -isystem /ITK/Modules/ThirdParty/ZLIB/src/itkzlib-ng -msimd128 -flto -Wno-warn-absolute-paths -DITK_WASM_NO_FILESYSTEM_IO  -O3 -DNDEBUG -std=c++20 -fPIC -Woverloaded-virtual -Wshadow -Wunused-parameter -MD -MT _deps/elx-build/Components/ImageSamplers/Full/CMakeFiles/FullSampler.dir/elxFullSampler.cxx.o -MF _deps/elx-build/Components/ImageSamplers/Full/CMakeFiles/FullSampler.dir/elxFullSampler.cxx.o.d -o _deps/elx-build/Components/ImageSamplers/Full/CMakeFiles/FullSampler.dir/elxFullSampler.cxx.o -c /work/emscripten-build/_deps/elx-src/Components/ImageSamplers/Full/elxFullSampler.cxx
  ...
/work/emscripten-build/_deps/elx-src/Common/ImageSamplers/itkImageFullSampler.hxx:113:12: error: no matching constructor for initialization of 'UserData'
  113 |   UserData userData{ inputImage, mask, GenerateWorkUnits(numberOfWorkUnits, croppedInputImageRegion, samples) };
      |            ^       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

So now I finally see the compiler command-line flags that I wanted to see. It has -std=c++20, is that necessary? With elastix, we are currently still at C++17, and we did not test C++20 before. Those compile errors do not appear with -std=c++17 either!

Looks like this is the breaking change between C++17 and C++20: p1008r1 - Prohibit aggregates with user-declared constructors.

@thewtex Can you please mention in your commit message that your adjustments are necessary for C++20 support?

thewtex commented 1 month ago

@N-Dekker great digging!

@thewtex Can you please mention in your commit message that your adjustments are necessary for C++20 support?

Yes, done.

This was with my local build on Debian Stable (Bullseye).

We do use C++20 with ITK-Wasm for glaze -- some lovely modern C++ there ! :sparkling_heart:

N-Dekker commented 1 month ago

Yes, done.

Thanks Matt, I will merge your PR as soon as the CI allows me to!

We do use C++20 with ITK-Wasm for glaze -- some lovely modern C++ there ! 💖

Cool.... but I see, it even required C++23, nowadays: https://github.com/stephenberry/glaze?tab=readme-ov-file#compilersystem-support I guess ITK-Wasm uses an older version of glaze, right?

thewtex commented 1 month ago

Thanks for the review and integration, Neils 🙏

Cool.... but I see, it even required C++23, nowadays: https://github.com/stephenberry/glaze?tab=readme-ov-file#compilersystem-support I guess ITK-Wasm uses an older version of glaze, right?

Yes, it was just added recently:

https://github.com/stephenberry/glaze/pull/1149

but it seems to be supported by the same compilers.

N-Dekker commented 1 month ago

@thewtex Should we then consider to upgrade ITK and elastix to C++20? 🤔

thewtex commented 1 month ago

@thewtex Should we then consider to upgrade ITK and elastix to C++20? 🤔

ITK may be difficult do to the community's needs to support older compilers. But we could do more testing / prep for ITK with elastix, ITK-Wasm, ...

N-Dekker commented 1 month ago

ITK may be difficult do to the community's needs to support older compilers.

May be, indeed. But for ITKElastix, both ITK and elastix now need to be C++20-compatible (= compilable), right?

thewtex commented 1 month ago

May be, indeed. But for ITKElastix, both ITK and elastix now need to be C++20-compatible (= compilable), right?

Yes. This is the case for ITK, and it should be the case for elastix with my patches. The ITK-Wasm build of both uses C++20.