WIP: Remove `virtual` from Get/Set macro definitions #2785

N-Dekker · 2021-10-07T10:23:22Z

Declared Get/Set member functions non-virtual by default when ITK_FUTURE_LEGACY_REMOVE is set.

Added itkVirtual and itkNonVirtual Get/Set macro's, to explicitly choose between virtual and non-virtual.

Observed a significant reduction in the size of generated "ITK*.lib" files when declaring Get/Set member functions non-virtual by default. For example, "ITKTransformFactory-5.3.lib" went from 18.3 MB down to 17.5 MB, and "ITKCommon-5.3.lib" went from 8.1 MB down to 7.6 MB, using VS2019 (Release).

N-Dekker · 2021-10-07T10:28:23Z

@jhlegarreta I see now, this PR partially overlaps with 10fce06 from your pull request #2568 "STYLE: Use macros to Set/Get ivars in Common operators"

jhlegarreta · 2021-10-07T13:31:38Z

@jhlegarreta

I did not want to change the name of the current macros to contain virtual in their names. This affects virtually all header files.

My proposal is to leave the existing macro calls unchanged, except for the very few that must be virtual, because they actually have an override (in a derived class). Only those that really need to be virtual should call the corresponding itkVirtual macro.

When does one know that a class will not be subclassed and thus the non-virtual macro should be used? If this estimation is inaccurate, and the class and the Set/Get methods need to be made virtual, we would fall into what this comment boldly discouraged/refused.

I think it is rarely necessary to override a Get or a Set member function. But it should really be an explicit design choice to do so. When would you override a Get or a Set member function? Honestly, I did encounter a few cases where a virtual Get or Set was expected: "COMP: Replace Get/Set macro calls by the corresponding itkVirtual call"

This should be made crystal clear in the ITK SW Guide (and I feel it cannot be left to criteria of the contributor/clear guidelines are required).

Agreed!

blowekamp · 2021-10-07T14:01:53Z

This change is not going to be backwards compatible, and is not an appropriate change to ITK API for a minor release.

I am concerned with the frequency of aggressive changes that are not backwards compatible being proposed.

N-Dekker · 2021-10-07T14:19:56Z

This change is not going to be backwards compatible, and is not an appropriate change to ITK API for a minor release.

Thanks @blowekamp but I hope we may still agree that this change is a step in the right direction 😃 Do you agree that given our experience over the last few years, declaring Set and Get member functions non-virtual by default is in principle the right thing to do?

It's not just an improvement of performance, and a reduction of the size of the lib files, it also reduces the code complexity. I mean:

Does ++m_CurrentIteration semantically do the same as:

this->SetCurrentIteration( this->GetCurrentIteration() + 1 );

?

When the Set and Get member functions are non-virtual, the answer is obviously yes. But what if they are virtual?

blowekamp · 2021-10-07T14:29:11Z

Do you agree that given our experience over the last few years, declaring Set and Get member functions non-virtual by default is in principle the right thing to do?

Broadly, across the whole toolkit without consideration for base classes and derived usages, I say: no. For that case of derived filter's "parameters" and other internal variables, maybe. However, one of the original goals of ITK's design was that developers and researchers could take the ITK implementation and derived a custom class with experimental or custom algorithm for changes improvements. Virtual setters and getters are needed for the appropriate polymorphic behavior.

Regarding the change is size of the libraries, what is the difference in size of the minsize build type? I believe this build type strips some of the strings of the names of the functions so it may be less of a difference.

dzenanz

I agree with Brad that this is somewhat of a big change. If doing it now, make it FUTURE_LEGACY, which would graduate into LEGACY at time of 6.0. And, of course, wait until 5.3 final is out before merging it.

For virtual dispatches to work correctly, the setters must be invoked consistently (instead of setting the ivars directly). Because there are not that many instances where virtual setters are overridden, the consistency in calling the setters is absent. So there is a case for making the setters non-virtual by default. Also, this is an open source library which is open to contributions. If someone needs some class' functionality to be virtualized, they can propose a PR which makes some or all of the setters virtual.

N-Dekker · 2021-10-08T09:06:46Z

@blowekamp

Regarding the change is size of the libraries, what is the difference in size of the minsize build type? I believe this build type strips some of the strings of the names of the functions so it may be less of a difference.

For a MinSizeRel VS2019 build, ITK lib files are also significantly smaller when removing virtual from the Get and Set macro's.

"ITKTransformFactory-5.3.lib" before removing virtual: 18.8 MB
"ITKTransformFactory-5.3.lib" after removing virtual: 17.9 MB
"ITKCommon-5.3.lib" before removing virtual: 9.37 MB
"ITKCommon-5.3.lib" after removing virtual: 8.92 MB

N-Dekker · 2021-10-08T10:33:09Z

@dzenanz

I agree with Brad that this is somewhat of a big change. If doing it now, make it FUTURE_LEGACY, which would graduate into LEGACY at time of 6.0.

OK, thanks Dženan. I just made the removal "ITK_FUTURE_LEGACY_REMOVE-only", with the latest force-push, please check!

dzenanz

LGTM

commit 8f26661 creates blob 12398f35ac335d57dafd510e044b91bd28b4d29f at Modules/Core/Common/include/itkMacro.h with size 101090 bytes (98.72 KiB) which is greater than the maximum size 100000 bytes (97.66 KiB). If the file is intended to be committed, set the hooks-max-size attribute on its path.

Wow, itkMacro.h is 100KB! This might be the default limit, so we will need a special entry in .gitattributes for it.

N-Dekker · 2021-10-08T14:44:00Z

LGTM

Thanks @dzenanz!

Wow, itkMacro.h is 100KB!

I think we can save a considerable amount of bytes by removing all those trailing spaces 😺 Look here, for example:

#define itkSimpleNewMacro(x)                                                                                           \
  static Pointer New()                                                                                                 \
  {                                                                                                                    \
    Pointer smartPtr = ::itk::ObjectFactory<x>::Create();                                                              \
    if (smartPtr == nullptr)                                                                                           \
    {                                                                                                                  \
      smartPtr = new x;                                                                                                \
    }                                                                                                                  \
    smartPtr->UnRegister();                                                                                            \
    return smartPtr;                                                                                                   \
  }                                                                                                                    \
 ITK_MACROEND_NOOP_STATEMENT

Could be reduced to:

#define itkSimpleNewMacro(x)                              \
  static Pointer New()                                    \
  {                                                       \
    Pointer smartPtr = ::itk::ObjectFactory<x>::Create(); \
    if (smartPtr == nullptr)                              \
    {                                                     \
      smartPtr = new x;                                   \
    }                                                     \
    smartPtr->UnRegister();                               \
    return smartPtr;                                      \
  }                                                       \
 ITK_MACROEND_NOOP_STATEMENT

Or maybe even:

#define itkSimpleNewMacro(x) \
  static Pointer New() \
  { \
    Pointer smartPtr = ::itk::ObjectFactory<x>::Create(); \
    if (smartPtr == nullptr) \
    { \
      smartPtr = new x; \
    } \
    smartPtr->UnRegister(); \
    return smartPtr; \
  } \
 ITK_MACROEND_NOOP_STATEMENT

What do you think?

As a last resort, we may also convert the indentation style from spaces to tabs 😺😺😺

dzenanz · 2021-10-08T14:48:46Z

Option 2 is reasonable, but does clang-format fight against it?

Also, I was not complaining. I was merely being wowed by how big that file has gotten 😄

Aims to fix: > commit 8f26661 creates blob [...] at Modules/Core/Common/include/itkMacro.h > with size 101090 bytes (98.72 KiB) which is greater than the maximum > size 100000 bytes (97.66 KiB). If the file is intended to be > committed, set the hooks-max-size attribute on its path. From pull request InsightSoftwareConsortium#2785 "Remove virtual from Get/Set macro definitions"

Aligns escaped (`\`) newlines as far left as possible, according to https://clang.llvm.org/docs/ClangFormatStyleOptions.html Applies especially to macro definitions. Aims to fix a kwrobot-v1/ghostflow-check-master error at pull request InsightSoftwareConsortium#2785 "Remove virtual from Get/Set macro definitions", as reported by Dženan Zukić: > commit 8f26661 creates blob [...] at Modules/Core/Common/include/itkMacro.h > with size 101090 bytes (98.72 KiB) which is greater than the maximum > size 100000 bytes (97.66 KiB). If the file is intended to be > committed, set the hooks-max-size attribute on its path.

Aligns escaped (`\`) newlines as far left as possible, according to https://clang.llvm.org/docs/ClangFormatStyleOptions.html Applies especially to macro definitions. Aims to fix a kwrobot-v1/ghostflow-check-master error at pull request InsightSoftwareConsortium#2785 "Remove virtual from Get/Set macro definitions", as reported by Dženan Zukić: > commit 8f26661 creates blob [...] at Modules/Core/Common/include/itkMacro.h > with size 101090 bytes (98.72 KiB) which is greater than the maximum > size 100000 bytes (97.66 KiB). If the file is intended to be > committed, set the hooks-max-size attribute on its path. Manually adjusted the six "clang-format off" sections in itkMacro.h according to the clang-format preferences.

Aligns escaped (`\`) newlines as far left as possible, according to https://clang.llvm.org/docs/ClangFormatStyleOptions.html Applies especially to macro definitions. Aims to fix a kwrobot-v1/ghostflow-check-master error at pull request #2785 "Remove virtual from Get/Set macro definitions", as reported by Dženan Zukić: > commit 8f26661 creates blob [...] at Modules/Core/Common/include/itkMacro.h > with size 101090 bytes (98.72 KiB) which is greater than the maximum > size 100000 bytes (97.66 KiB). If the file is intended to be > committed, set the hooks-max-size attribute on its path. Manually adjusted the six "clang-format off" sections in itkMacro.h according to the clang-format preferences.

Declared the member functions defined by Get/Set macro's non-virtual, when `ITK_FUTURE_LEGACY_REMOVE` is set. (Otherwise those member functions will still be `virtual`.)

Allowed specifying explicitly whether or not a Get/Set member function should be declared `virtual`.

Only for those Get/Set member functions that actually have an override.

stale · 2022-04-16T09:34:28Z

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

hjmjohnson · 2024-05-31T15:11:42Z

@N-Dekker After years in the making, this is finally ready to be updated (rebased) and reconsidered!

hjmjohnson · 2024-11-11T15:18:04Z

@N-Dekker Now that we are at ITK 6.0, can we revisit this? It seems like now is the time to put effort into evaluating this effort.

N-Dekker · 2024-11-11T16:47:25Z

Now that we are at ITK 6.0, can we revisit this?

@hjmjohnson I have to think about it. Over the last few years (after submitting this PR) I did encounter some cases where Get and/or Set member functions were overridden. I don't really like that, I think it's error prone. (What if someone overrides the Get, but forgets to override the corresponding Set?) And when a class has a specific data member allocated for a Get and Set, it seems wasteful to override it to bypass the allocated data. But having said so, I know now that this feature of overridable Get and Set is actually being used in practice. So I hesitate. 🤔

hjmjohnson · 2024-11-11T18:18:36Z

im just revisiting old PRs Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Niels Dekker ***@***.***> Sent: Monday, November 11, 2024 10:47:47 AM To: InsightSoftwareConsortium/ITK ***@***.***> Cc: Hans Johnson ***@***.***>; Mention ***@***.***> Subject: Re: [InsightSoftwareConsortium/ITK] Remove `virtual` from Get/Set macro definitions (#2785) Now that we are at ITK 6.0, can we revisit this? @hjmjohnson<https://github.com/hjmjohnson> I have to think about it. Over the last few years (after submitting this PR) I did encounter some cases where Get and/or Set member functions were overridden. I don't really like that, I think it's error prone. (What if someone overrides the Get, but forgets to override the corresponding Set?) And when a class has a specific data member allocated for a Get and Set, it seems wasteful to override it to bypass the allocated data. But having said so, I know now that this feature of overridable Get and Set is actually being used in practice. So I hesitate. 🤔 — Reply to this email directly, view it on GitHub<#2785 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AACMU4QGUJ5JSK2LB5SQSU32ADNTHAVCNFSM6AAAAABRSB4MTCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRYGYYTINJXGA>. You are receiving this because you were mentioned.Message ID: ***@***.***>

dzenanz · 2025-01-24T19:17:21Z

Maybe mark this PR as draft until you make up your mind?

N-Dekker requested a review from jhlegarreta October 7, 2021 10:23

N-Dekker changed the title ~~Remove virtual from Get/Set macros~~ Remove virtual from Get/Set macro definitions Oct 7, 2021

N-Dekker force-pushed the Remove-virtual-from-Get-and-Set-macros branch 2 times, most recently from 816b8b4 to 832c78f Compare October 7, 2021 13:21

dzenanz reviewed Oct 7, 2021

View reviewed changes

dzenanz mentioned this pull request Oct 7, 2021

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

Open

2 tasks

N-Dekker force-pushed the Remove-virtual-from-Get-and-Set-macros branch from 832c78f to 6cd61e1 Compare October 8, 2021 10:25

dzenanz reviewed Oct 8, 2021

View reviewed changes

N-Dekker mentioned this pull request Oct 8, 2021

STYLE: clang-format AlignEscapedNewlines Left (for macro definitions) #2790

Merged

N-Dekker force-pushed the Remove-virtual-from-Get-and-Set-macros branch from 6cd61e1 to 970aa45 Compare October 13, 2021 16:24

N-Dekker marked this pull request as ready for review October 13, 2021 21:15

thewtex added this to the ITK 6.0.0 milestone Oct 20, 2021

N-Dekker mentioned this pull request Oct 21, 2021

STYLE: Remove redundant overrides of Get and Set member functions #2833

Merged

N-Dekker added 3 commits October 26, 2021 00:18

ENH: FUTURE_LEGACY_REMOVE Remove virtual from Get/Set macro definitions

aa81599

Declared the member functions defined by Get/Set macro's non-virtual, when `ITK_FUTURE_LEGACY_REMOVE` is set. (Otherwise those member functions will still be `virtual`.)

ENH: Add itkVirtual and itkNonVirtual Get/Set macro's

27adc4b

Allowed specifying explicitly whether or not a Get/Set member function should be declared `virtual`.

COMP: Replace Get/Set macro calls by the corresponding itkVirtual call

21d3a54

Only for those Get/Set member functions that actually have an override.

N-Dekker force-pushed the Remove-virtual-from-Get-and-Set-macros branch from 970aa45 to 21d3a54 Compare October 25, 2021 22:21

N-Dekker mentioned this pull request Feb 19, 2022

Gdcm update 2022 01 05 #3042

Closed

stale bot added the status:Use_Milestone_Backlog Use "Backlog" milestone instead of label for issues without a fixed deadline label Apr 16, 2022

hjmjohnson removed the status:Use_Milestone_Backlog Use "Backlog" milestone instead of label for issues without a fixed deadline label Apr 17, 2022

N-Dekker mentioned this pull request Oct 21, 2022

Add TransformixFilter::SetTransform SuperElastix/elastix#747

Merged

hjmjohnson modified the milestones: ITK 6.0.0, ITK 6.0 Beta 1 Jan 24, 2025

N-Dekker marked this pull request as draft January 26, 2025 21:12

N-Dekker changed the title ~~Remove virtual from Get/Set macro definitions~~ WIP: Remove virtual from Get/Set macro definitions Jan 26, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Remove `virtual` from Get/Set macro definitions #2785

WIP: Remove `virtual` from Get/Set macro definitions #2785

N-Dekker commented Oct 7, 2021 •

edited

Loading

N-Dekker commented Oct 7, 2021

jhlegarreta commented Oct 7, 2021 •

edited by N-Dekker

Loading

blowekamp commented Oct 7, 2021

N-Dekker commented Oct 7, 2021

blowekamp commented Oct 7, 2021

dzenanz left a comment

N-Dekker commented Oct 8, 2021

N-Dekker commented Oct 8, 2021

dzenanz left a comment

N-Dekker commented Oct 8, 2021

dzenanz commented Oct 8, 2021

stale bot commented Apr 16, 2022

hjmjohnson commented May 31, 2024

hjmjohnson commented Nov 11, 2024

N-Dekker commented Nov 11, 2024

hjmjohnson commented Nov 11, 2024 via email

dzenanz commented Jan 24, 2025

WIP: Remove virtual from Get/Set macro definitions #2785

Are you sure you want to change the base?

WIP: Remove virtual from Get/Set macro definitions #2785

Conversation

N-Dekker commented Oct 7, 2021 • edited Loading

N-Dekker commented Oct 7, 2021

jhlegarreta commented Oct 7, 2021 • edited by N-Dekker Loading

blowekamp commented Oct 7, 2021

N-Dekker commented Oct 7, 2021

blowekamp commented Oct 7, 2021

dzenanz left a comment

Choose a reason for hiding this comment

N-Dekker commented Oct 8, 2021

N-Dekker commented Oct 8, 2021

dzenanz left a comment

Choose a reason for hiding this comment

N-Dekker commented Oct 8, 2021

dzenanz commented Oct 8, 2021

stale bot commented Apr 16, 2022

hjmjohnson commented May 31, 2024

hjmjohnson commented Nov 11, 2024

N-Dekker commented Nov 11, 2024

hjmjohnson commented Nov 11, 2024 via email

dzenanz commented Jan 24, 2025

WIP: Remove `virtual` from Get/Set macro definitions #2785

WIP: Remove `virtual` from Get/Set macro definitions #2785

N-Dekker commented Oct 7, 2021 •

edited

Loading

jhlegarreta commented Oct 7, 2021 •

edited by N-Dekker

Loading