Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

COMP: Use default copy and move construct and assign in itk::Region #4627

Conversation

jhlegarreta
Copy link
Member

@jhlegarreta jhlegarreta commented Apr 29, 2024

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
Copy link
Member Author

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

@github-actions github-actions bot added type:Compiler Compiler support or related warnings area:Core Issues affecting the Core module labels Apr 29, 2024
@seanm
Copy link
Contributor

seanm commented Apr 29, 2024

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

@N-Dekker
Copy link
Contributor

N-Dekker commented Apr 30, 2024

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 jhlegarreta force-pushed the DisallowCopyAndMoveForRegion branch 3 times, most recently from 43ad378 to 7eba4b2 Compare April 30, 2024 22:42
@jhlegarreta
Copy link
Member Author

#4627 (comment) 👍 Done. Thanks Niels.

@jhlegarreta jhlegarreta changed the title COMP: Use ITK_DISALLOW_COPY_AND_MOVE in itk::Region COMP: Use default copy and move construct and assign in itk::Region Apr 30, 2024
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
@jhlegarreta
Copy link
Member Author

/azp run ITK.Windows

Copy link
Contributor

@N-Dekker N-Dekker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it indeed fix the warnings? I guess so, but you never know... anyway, approved, of course. 👍 Thanks Jon!

@jhlegarreta jhlegarreta merged commit 776e1dc into InsightSoftwareConsortium:master May 2, 2024
14 checks passed
@jhlegarreta jhlegarreta deleted the DisallowCopyAndMoveForRegion branch May 2, 2024 00:58
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request May 8, 2024
Instead of "manually" defaulting their copy and move member functions.

Follow-up to two commits by Jon Haitz Legarreta Gorroño:

pull request InsightSoftwareConsortium#4626
commit 9aff210
"COMP: Use default copy, move for construc, assign `RealTimeStamp`"

pull request InsightSoftwareConsortium#4627
commit ec2677f
"COMP: Use `default` copy and move construct and assign in `itk::Region`"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request May 13, 2024
Instead of "manually" defaulting their copy and move member functions.

Follow-up to two commits by Jon Haitz Legarreta Gorroño:

pull request InsightSoftwareConsortium#4626
commit 9aff210
"COMP: Use default copy, move for construc, assign `RealTimeStamp`"

pull request InsightSoftwareConsortium#4627
commit ec2677f
"COMP: Use `default` copy and move construct and assign in `itk::Region`"
hjmjohnson pushed a commit that referenced this pull request May 15, 2024
Instead of "manually" defaulting their copy and move member functions.

Follow-up to two commits by Jon Haitz Legarreta Gorroño:

pull request #4626
commit 9aff210
"COMP: Use default copy, move for construc, assign `RealTimeStamp`"

pull request #4627
commit ec2677f
"COMP: Use `default` copy and move construct and assign in `itk::Region`"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Core Issues affecting the Core module type:Compiler Compiler support or related warnings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants