STYLE: Use macros to Set/Get ivars in `Common` operators

[jhlegarreta]

Use macros to Set/Get ivars in Common operators.

Take advantage of the commit to leave the documentation of the ivars only in their corresponding Set/Get methods, as dictated by the ITK SW Guide.

PR Checklist

• [X] No API changes were made (or the changes have been approved)
• [X] No major design changes were made (or the changes have been approved)

[jhlegarreta]

Not sure about what the errors imply: https://open.cdash.org/viewBuildError.php?buildid=7260537

Suggestions are welcome.

[jhlegarreta]

Push forced to see the error reports back and see where we were at this level.

[N-Dekker]

I'm sorry but I still don't like to have a virtual keyword added to those Get and Set member functions. Especially when they were originally non-virtual, and when they worked just fine without a virtual keyword. The only reason for adding virtual to an existing member function should be that some user actually needs to override it.

C++ Core Guidelines C.132: Don’t make a function virtual without reason says on this topic:

Redundant virtual increases run-time and object-code size. A virtual function can be overridden and is thus open to mistakes in a derived class...

We already spent a lot of time in the last few years, removing virtual from the ITK code base, and we're still not finished. Removal of virtual might break legacy user code, even while it is an improvement to new user code, which is why Bradley (@blowekamp) introduced a ITK_ITERATOR_VIRTUAL macro, for legacy support.

https://github.com/InsightSoftwareConsortium/ITK/blob/6e3d31d138ff48ae6e5f2e32fcb17c5f172a29ed/Modules/Core/Common/include/itkMacro.h#L1314-L1326

So please only make a member function virtual when it is actually needed!

[jhlegarreta]

Thanks for the comment @N-Dekker . Then, as pointed in this comment of yours I'll mark it to status:Blocked until the proposed itkGetConstNonVirtualMacro gets added to the ITK code base.

[jhlegarreta]

10fce06 might go somehow unnoticed inside the PR and unless one takes a look at the commit history, but did not want to push a PR that added code that was not exercised. Also, I am not sure if all these would get extensive use with time. Open to suggestions.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

[hjmjohnson]

@jhlegarreta Is this still something that needs to be worked on? If so, could you please rebase and make a game plan for what needs to be done?

[jhlegarreta]

@jhlegarreta Is this still something that needs to be worked on? If so, could you please rebase and make a game plan for what needs to be done?

Thanks for the heads-up Hans. Still on my ToDo list; have not had the time too look at my ITK-related ToDo list in a while. I do think this still deserves having a go. Not sure when I'll be able to take it up, though.

Also, wisest might be to wait until what is mentioned here is resolved/done: https://github.com/InsightSoftwareConsortium/ITK/pull/2568#discussion_r724308177