Defaulting copy constructor, copy assignment, move constructor, and move assignment functions

[jhlegarreta]

Description

The dashboard site RogueResearch22 is consistently raising warnings about classes declaring implicit copy constructors, copy assignment operators, etc. https://learn.microsoft.com/bs-latn-ba/cpp/error-messages/compiler-warnings/c5267?view=msvc-150#remarks

Over the last few days some PRs have addressed some of these warnings: PR #4626, #4627, #4639

However, every time part of these warnings are fixed, more warnings are raised due to related issues (looks like only 199 warnings can be/are reported at a time to the dashboard).

Impact analysis

Using the ITK_DISALLOW_COPY_AND_MOVE macro does not work for these cases, since they need to allow creating copies, etc.

Thus, these classes contradict what the ITK_DISALLOW_COPY_AND_MOVE macro says about ITK's design. Going forward, probably that needs to be revisited, and a new macro be defined so that defaulting the copy constructor, copy assignment, move constructor, and move assignment functions is done with a single command, and thus saving typing. Maybe multiple macros need to be declared depending on the case, or a macro that defaults some of the cases depending on some parameter.

Expected behavior

No warnings are raised.

Actual behavior

199+ warnings are raised by RogueResearch22.

Versions

OS: Mac10.13 Compiler: AppleClang-rel-x86_64 ITK: master

Additional Information

These implicit declarations now raise warnings, but may be removed in the future: https://learn.microsoft.com/bs-latn-ba/cpp/error-messages/compiler-warnings/c5267?view=msvc-150#remarks

[jhlegarreta]

CC @seanm

[seanm]

I didn't read it all, but how is the google doc link relevant?

[jhlegarreta]

Sorry @seanm, shared the wrong link. Fixed.

[seanm]

Ah! No worries, I was confused there :)

[N-Dekker]

Thanks for raising this issue, Jon! Maybe we should introduce a new macro, e.g., ITK_DEFAULT_COPY_AND_MOVE:

#define ITK_DEFAULT_COPY_AND_MOVE(TypeName)         \
  TypeName(const TypeName &) = default;             \
  TypeName & operator=(const TypeName &) = default; \
  TypeName(TypeName &&) = default;                  \
  TypeName & operator=(TypeName &&) = default

Similar to

https://github.com/InsightSoftwareConsortium/ITK/blob/4926c2fc80272646976299a00de33b35cf53b022/Modules/Core/Common/include/itkMacro.h#L388-L392

I think we should still follow the rule of zero "by default". But in cases where is is necessary to have a user-declared destructor, a macro like ITK_DEFAULT_COPY_AND_MOVE could be useful. What do you think?

[jhlegarreta]

Sounds reasonable. Probably should be documented (e.g. SW Guide) as well: i.e. generally in which cases we will prefer to disallow (e.g. filters?) vs default (e.g. images, regions?). Is there some case where only some of the statements (e.g. only the constructor vs the assignment) will need to be defaulted a priori?

[N-Dekker]

For the moment (at least for ITK 5.4.0), as a guideline, I would only just use the new ITK_DEFAULT_COPY_AND_MOVE (PR #4652) when necessary to fix those warnings.

In general, copying and moving should only be enabled when it "makes sense", for the specific type. But for the moment, I think we should avoid disallowing copy and move for existing classes (adding extra DISALLOW macro calls), because doing so would be an API change.

Adding ITK_DEFAULT_COPY_AND_MOVE to a class that was originally implicitly copyable and movable is OK now, I think, because it's not an API change.

[jhlegarreta]

Sounds good to me Niels. Thanks.