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: Use default copy and move construct and assign in `itk::Region` #4627

Closed jhlegarreta closed 5 months ago

jhlegarreta commented 5 months ago

Use the compiler-proved default implementations for itk::Region copy constructor, copy assignment, move constructor, and move assignment functions.

As noted in [1], the C++ standard deprecated the implicit generation of copy and assignment operators.

Fixes:

[CTest: warning matched] /Users/builder/externalModules/Core/Common/include/itkRegion.h:91:11:
 warning: definition of implicit copy assignment operator for 'Region' is deprecated because it has a user-declared destructor [-Wdeprecated]
  virtual ~Region() = default;
          ^
[CTest: warning matched] /Users/builder/externalModules/Core/Common/include/itkImageIORegion.h:117:30:
 note: in implicit copy assignment operator for 'itk::Region' first required here
  operator=(Self &&) = default;
                             ^
[CTest: warning matched] /Users/builder/externalModules/IO/ImageBase/include/itkImageIOBase.h:228:3:
 note: in implicit move assignment operator for 'itk::ImageIORegion' first required here
  itkSetMacro(IORegion, ImageIORegion);
  ^
[CTest: warning matched] /Users/builder/externalModules/Core/Common/include/itkMacro.h:992:22:
 note: expanded from macro 'itkSetMacro'
      this->m_##name = std::move(_arg);                        \
                     ^

And other similar warnings stemming from itk::Region that have been appearing consistently in some macOS site builds in the dashboard: https://open.cdash.org/viewBuildError.php?type=1&buildid=9579479

[1] https://learn.microsoft.com/bs-latn-ba/cpp/error-messages/compiler-warnings/c5267?view=msvc-150#remarks

PR Checklist

jhlegarreta commented 5 months ago

@seanm Hopefully this will remove some of the 199 warnings on RogueResearch22.

seanm commented 5 months ago

Cool, thanks! I defer to C++ experts on the correctness of this change, but seems reasonable to me!

N-Dekker commented 5 months ago

I'm sorry I think Region should remain copyable, as long as it is being used as base class of ImageRegion 🤷

It's a bit cumbersome, but I guess the warnings will go away by adding four of those "explicitly defaulted" member functions to the public section:

Region(const Region &) = default;
Region(Region &&) = default;
Region& operator=(const Region &) = default;
Region& operator=(Region &&) = default;
jhlegarreta commented 5 months ago

https://github.com/InsightSoftwareConsortium/ITK/pull/4627#issuecomment-2085575522 :+1: Done. Thanks Niels.

jhlegarreta commented 5 months ago

/azp run ITK.Windows