SimpleType m_Var = initialValue;

Engineering
1

Should we move away from the practice that the member variables are initialized in the constructor, and rather move them to the declaration? At least for simple types. A “go to declaration” would also inform the programmer about member’s default value, not just its type and containing class.

3 Likes
2

Interesting. :slight_smile: When initializing data members of a class this way (using C++11 in-class member initializers), it appears that their initial values are included automatically with the Doxygen generated documentation of the class! So they are self-documented!

So apparently, in-class member initialization reduces the chance of having mismatches between documentation and code (like this issue: Mismatch documentation and code SyNImageRegistrationMethod GaussianSmoothingVarianceForTheUpdateField default value).

1 Like
3

From the outlined reasons, this looks a nice feature to me, if there are no downsides at least.

I guess not every data type may be initialized this way, but in any case, looks reasonable to consider it.

Also, and since this has arised a few times lately, if this somehow impacts performance vs using initialization lists due to some reason that I ignore right now, then we may need to be more cautious.

Finally, I guess the script using some regex to spot the initializations/assignment of member variables and move them to the declaration would not be that easy to develop. But may both spot uninitialized variables (much like using the EXERCISE_BASIC_OBJECT_METHODS macro does in tests), as well as miss/identify others due to incorrect naming. So definitely a huge effort. But it may be a worthwhile one.

2 Likes
4

Thanks for your encouraging reply, Jon.

Maybe clang-tidy could do the job for us :grinning:
https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-default-member-init.html

Unfortunately I don’t have clang properly installed yet, on my Windows pc, and I’m not so familiar with clang anyway.

1 Like
5

This might be a job for our resident clang-tidy master @hjmjohnson! Like he doesn’t have enough to do already :smiley:

1 Like
6

Indeed… I believe Hans can do it :grin:

In the mean time I did have another look. It appears that by default, the clang-tidy option modernize-use-default-member-init uses curly braces:

SimpleType m_Var{initialValue};

@dzenanz Is that OK to you as well? Otherwise the UseAssignment option has to be enabled.

Still I haven’t got clang-tidy-fix working properly here, especially passing this UseAssignment option seems non-obvious to me. So it would be great if Hans could do it. :slight_smile:

1 Like
7

Do curly braces appear in Doxygen as default values too? Otherwise I don’t have a preference.

8

Yes they do :smiley: I just tried my local Doxygen installation (1.8.14), and it does display the initial values of the data members, in the generated HTML.

In general, curly braced initialization has more compile-time safety checking than the “assignment style” syntax. So maybe we should go for curly braces anyway.

9

Challenge accepted. Initial conversion is underway. PR to arrive shortly.

4 Likes
10

Great job, Hans! Thanks a lot!

P.S. It appears that the commit added a few unnecessary empty lines to constructor implementations: https://github.com/InsightSoftwareConsortium/ITK/pull/324/commits/a9a501e6cd7186671d5c47ab06ddc07d2cc126c7

Doesn’t matter much, of course. But could this be a clang-tidy bug? :upside_down_face:

1 Like