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, move for construc, assign RealTimeStamp #4626

Conversation

jhlegarreta
Copy link
Member

@jhlegarreta jhlegarreta commented Apr 29, 2024

Use the compiler-proved default implementations for itk::RealTimeStamp 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/itkRealTimeStamp.h:56:3:
 warning: definition of implicit copy assignment operator for 'RealTimeStamp' is deprecated because it has a user-declared destructor [-Wdeprecated]
  ~RealTimeStamp();
  ^
[CTest: warning matched] /Users/builder/externalModules/Core/Common/include/itkDataObject.h:452:3:
 note: in implicit copy assignment operator for 'itk::RealTimeStamp' first required here
  itkSetMacro(RealTimeStamp, RealTimeStamp);
  ^
[CTest: warning matched] /Users/builder/externalModules/Core/Common/include/itkMacro.h:992:22:
 note: expanded from macro 'itkSetMacro'
      this->m_##name = std::move(_arg);                        \
                     ^
[CTest: warning suppressed] 1 warning generated.

And other similar warnings stemming from itk::RealTimeStamp 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

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

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

@dzenanz
Copy link
Member

dzenanz commented Apr 30, 2024

Perhaps the constructor needs to be removed and explicitly or implicitly defaulted?

@jhlegarreta jhlegarreta force-pushed the DisallowCopyAndMoveForRealTimeStamp branch from 9cf3a4c to e4658ec Compare April 30, 2024 22:41
@jhlegarreta jhlegarreta changed the title COMP: Use ITK_DISALLOW_COPY_AND_MOVE in itk::RealTimeStamp COMP: Use default copy, move for construc, assign RealTimeStamp Apr 30, 2024
@jhlegarreta jhlegarreta force-pushed the DisallowCopyAndMoveForRealTimeStamp branch 2 times, most recently from 831dec1 to 47a4d27 Compare April 30, 2024 23:33
Use the compiler-proved default implementations for `itk::RealTimeStamp`
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/itkRealTimeStamp.h:56:3:
 warning: definition of implicit copy assignment operator for 'RealTimeStamp' is deprecated because it has a user-declared destructor [-Wdeprecated]
  ~RealTimeStamp();
  ^
[CTest: warning matched] /Users/builder/externalModules/Core/Common/include/itkDataObject.h:452:3:
 note: in implicit copy assignment operator for 'itk::RealTimeStamp' first required here
  itkSetMacro(RealTimeStamp, RealTimeStamp);
  ^
[CTest: warning matched] /Users/builder/externalModules/Core/Common/include/itkMacro.h:992:22:
 note: expanded from macro 'itkSetMacro'
      this->m_##name = std::move(_arg);                        \
                     ^
[CTest: warning suppressed] 1 warning generated.
```

And other similar warnings stemming from `itk::RealTimeStamp` 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 jhlegarreta force-pushed the DisallowCopyAndMoveForRealTimeStamp branch from 47a4d27 to 9aff210 Compare April 30, 2024 23:35
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.

the C++ standard deprecated the implicit generation of copy and assignment operators.

Just for the sake of completeness, in general, the "rule of zero" is still allowed (even though it assumes implicit generation of copy and assignment operators). But when following the rule of zero, the destructor should also be "implicit".

Thanks Jon, approved 👍

@jhlegarreta jhlegarreta merged commit 7a155f3 into InsightSoftwareConsortium:master May 2, 2024
14 checks passed
@jhlegarreta jhlegarreta deleted the DisallowCopyAndMoveForRealTimeStamp 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.

4 participants