STYLE: Add in-class {} initializers to classes with virtual functions

[N-Dekker]

Added in-class {} default member initializers to data members of classes that have one or more virtual member functions.

For an object that has one or more virtual member functions, the performance cost of adding a {} default member initializer to each of its non-static data members should be acceptable, relative to cost of the initialization of its virtual table.

Follow-up to pull request #3851 commit 5e2c49f "STYLE: Add in-class {} member initializers to objects created by New()"

[dzenanz]

How did you find all these classes?

[N-Dekker]

How did you find all these classes?

My script (in C++): https://gist.github.com/N-Dekker/738d28b8d7528a32c12e7a129160b00f

It looks for a line of code that start with virtual. If such a line is found, from there, it adds {} initializers to data members it encounters, until it encounters a line that starts with }; (Because }; means the end of class!)

[dzenanz]

Should this script be added to the repository?

[N-Dekker]

Should this script be added to the repository?

Maybe, but I don't want to maintain the script forever. Should the script itself then need to conform to the ITK coding guidelines?

[dzenanz]

I don't think so. It is a repository of scripts which were used once upon a time, and are therefore good starting points if you need the same thing again. You should add a short usage in the comments at the top of the script, so a future you/me/somebody else can make sense of what the script is for and how to invoke it.

[N-Dekker]

FYI, The force-push that I just did has included classes that have an override keyword.

[dzenanz]

Label PRs action build failure is unrelated to this PR.

[N-Dekker]

Thanks for your approval, @dzenanz

One of the concerns I have about adding the script to github.com/InsightSoftwareConsortium/ITK.git is that it would trigger another CI run with each revision. Which seems like a waste of energy to me, because the CI does not test the script at all. Would it be possible for the CI to bypass changes in ITK/Utilities/Maintenance ?

[dzenanz]

I don't know whether it is possible to run the CI conditionally (I don't think it is), and if so how to do it.

[N-Dekker]

I don't know whether it is possible to run the CI conditionally (I don't think it is), and if so how to do it.

Oh wait, I see now at https://learn.microsoft.com/en-us/azure/devops/pipelines/yaml-schema/trigger?view=azure-pipelines#trigger-batch-branches-paths-tags

trigger:
  batch: true
  branches:
    include:
    - features/*
    exclude:
    - features/experimental/*
  paths:
    exclude:
    - README.md

So it seems possible to exclude "README.md" from the CI trigger. Then also any other file, I guess.

[dzenanz]

You could give it a try, exclude Utilities/Maintenance before submitting a PR with the new script.