WIP: Remove virtual from Get/Set macro definitions

[N-Dekker]

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]

@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]

@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]

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]

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]

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]

@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]

@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]

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]

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 😄

[stale]

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

[hjmjohnson]

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

[hjmjohnson]

@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]

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]

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]

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

[hjmjohnson]

@N-Dekker, with the impending updates for moving towards ITK 6.0, this PR should be reconsidered and either prioritized for ITK v6.0, rejected, or pushed back to ITK v7.

This work is related to several other outstanding PRs. Perhaps we need to have a larger discussion about the pros/cons of potential solutions.

[hjmjohnson]

Per the request to consolidate the scattered virtual / override / final design conversation around ITK Get/Set macros, I've opened #6232 (Master tracking — virtual / override / final semantics on Get/Set macros for ITK 6.x) as a single venue.

It pulls in the 2018 Discourse #814 precedent (the same final-as-stress-test technique and the @phcerdan / @N-Dekker 18–24% HoughTransform performance gain), the 2026-05-07 final-keyword audit (17 base methods overridden across 19 files in upstream ITK), and a draft additive itkVirtualSetMacro family on hjmjohnson:macro-optionally-virtual @ ffc70ed0b9. Surfaces 7 binary/short-list questions — Q1 (default-emit virtual?) is the load-bearing one this PR is blocked on.

Suggest holding further design comments on this thread until #6232 has a maintainer-quorum answer on Q1; otherwise the conversation re-fragments. This thread can stay open as the implementation venue once direction is settled.