STYLE: Add in-class {} member initializers to objects created by New()

[N-Dekker]

For an object created by New(), the performance cost of adding a {} default member initializer to each of its non-static data members should be neglectable compared to the cost of allocating the object itself, as it is allocated on the heap.

In-class default member initialization effectively prevents Valgrind/Memcheck warnings like "Conditional jump or move depends on uninitialised value(s)", and Visual C++ Code Analysis warnings like "warning C26495: Variable is uninitialized. Always initialize a member variable (type.6)"

These cases are found using the regular expression \w.* m_\w[^{}=]+;, excluding static data members and data members of a reference (&) type.

[dzenanz]

Wouldn't this make it harder to spot a failure to really initialize these ivars in the constructor, or elsewhere?

[N-Dekker]

Wouldn't this make it harder to spot a failure to really initialize these ivars in the constructor, or elsewhere?

Good question, but I'm not sure... Suppose a numeric data member is just zero-initialized by {}, while it should have been initialized to non-zero by the constructor, wouldn't that be easy to spot? Suppose a raw pointer is initialized as nullptr by {}, while it should have been initialized to a valid address in the constructor. Wouldn't that be easy to spot?

The use of uninitialized data isn't always spotted automatically, it may just have undefined behavior silently.

[N-Dekker]

Instead of a bulk commit like this, we could just keep chasing uninitialized member variables one by one, as in PR #3845 (by Jon) #3843, and #3849 (by myself). Which is certainly useful, but also very time consuming. And we probably won't fix all of them that way, one by one. I think it's preferable to just initialize all data members by default. And only leave some particular data uninitialized in very special (performance critical) cases. What do you think?

[dzenanz]

I have mildly positive attitude towards this PR. I would like some other people to pitch in.

[N-Dekker]

ITK.Linux has some warning at https://open.cdash.org/viewBuildError.php?type=1&buildid=8377051, but it seems unrelated to this PR, I don't know:

Modules/Core/Common/include/itkOctree.hxx:32:1: warning: const/copy propagation disabled: 49167 basic blocks and 82024 registers; increase --param max-gcse-memory above 504256752 [-Wdisabled-optimization]

[N-Dekker]

FYI Here's a cheat sheet for the value initialized by {}:

Type	Value initialized by {}
numeric type (int, unsigned, float, etc.)	zero
bool	false
pointer type	nullptr
class that has a user-provided default-constructor	default-constructed object
array	all elements value-initialized (as if by {})
aggregate (struct)	all data members value-initialized (as if by {})

[N-Dekker]

/azp run ITK.Linux

[N-Dekker]

For the record, here is the discussion topic that I started on this subject: Adding in-class {} default member initializers to data members of ITK classes

The script I wrote to do the code changes is at https://gist.github.com/N-Dekker/738d28b8d7528a32c12e7a129160b00f

[thewtex]

👏 ✨

[jhlegarreta]

Thanks for having taken the time to do this Niels.

A couple of in-line comments.

Please:

• Translate the gist (:ok_hand:) into a script file so that it dwells in the ITK Maintenance Utilities. Remotes may be grateful at some point.
• Apply this to non-default modules and examples (if not already done).
• Write this in the ITK SW guide.

[dzenanz]

Give a few more days for others to comment/review, then merge? But corresponding update should go into the software guide either concurrently with the merge of this, or very soon afterwards.

[N-Dekker]

Thanks for your feedback @dzenanz

Give a few more days for others to comment/review, then merge?

Sure, that's fine to me!

But corresponding update should go into the software guide either concurrently with the merge of this, or very soon afterwards.

Actually, it already says:

All member variables must be initialized when they are declared.

https://github.com/InsightSoftwareConsortium/ITKSoftwareGuide/blob/905c9bbc525adc73ff934d0ed24d06ef11151e22/SoftwareGuide/Latex/Appendices/CodingStyleGuide.tex#L1568

I just made a pull request to no longer exclude smart pointers. Instead, it proposes to exclude low level utility classes. Please check: InsightSoftwareConsortium/ITKSoftwareGuide#192

[N-Dekker]

For your information, the last force-push just did another git rebase master, and solved the one little merge conflict.