DOC: Remove mistaken exception to data member initialization guideline

[N-Dekker]

When a data member is set by a "setter" member function (Set##name, On, or Off), it is still recommended to initialize the member according to this guideline. Doing so prevents problems like the one addressed by pull request InsightSoftwareConsortium/ITK#3845 commit InsightSoftwareConsortium/ITK@a31e8bf "BUG: Fix uninitialized value ImageRegistrationMethodv4::m_NumberOfLevels" by Jon Haitz Legarreta Gorroño (@jhlegarreta), 10 January 2023

[N-Dekker]

Thanks for your approval, @dzenanz. I'm so happy the CI accepts this pull request of mine as well 😃

For the record, this topic was already discussed at #192 (comment) This pull request would revert d07be39

[jhlegarreta]

I am unsure about this

• I do believe that what Niels is proposing is useful and helps to avoid uninitialized warnings.

• The SWG is becoming inconsistent concerning initialization: the related passages were written at a time where members were initialized in the constructor (and I have a feeling that Examples that fill the SWG do not receive these updates), as opposed to the in-class/header brace initialization that is being encouraged lately.

  In the former case, it did not make sense to initialize a member in the constructor initialization list to immediately change it in the body. Now, with the in-class/header brace initialization, all is initialized in the header to some zero/default values, and then members are adjusted in the constructor body (precisely following the case mentioned in the related PR, if e.g. the number of levels is changed to 4, all array initializations should also initialize a new element, and that is best managed within a Set## or Initialize## method to avoid bugs IMO).

My take on this is that

• this is unclear, or
• I am biased and I tend to over-explain, or
• Niels' proposal is perfectly compatible with the above.

So please feel free to merge it.

[N-Dekker]

Thanks for your comment Jon. I would still like to mention that a call to a setter or initialize member function in the default-constructor does not call an override of a possible derived class, even when this setter or initialize member function is declared virtual. For example:

class Base
{
public:
  Base()
  {
    SetX(42); // Does not call Derived::SetX, even though it is virtual !!!
  }

  virtual void
  SetX(int);

private:
  int m_X{};
};

class Derived : public Base
{
  void
  SetX(int) override;
};

Why not? That's just how C++ works. It does not use the virtual function table of an object during its construction.

I'm not sure if that was clear to everyone back in 2018 when you originally added the text, commit d07be39 Especially because the text suggests that the setter may be "overloaded by some classes in the future".

[jhlegarreta]

#196 (comment) 🙃 thanks Niels, as always for this mastery. Not sure if we've relied on this/how frequent is the case in ITK. But it comes down to the derived class programmer the job of taking care of this: in the case at issue (SetNumberOfLevels) (regardless the method name, e.g. using an Initialize method), it seems to me that at times grouping related initializations in a method may be useful.

[N-Dekker]

So please feel free to merge it.

@jhlegarreta Thanks Jon, but I don't have the permission to merge at the SoftwareGuide. (And I'd rather not merge my own pull requests anyway.) So I can just repeat: please feel free to merge it 😃

Not sure if we've relied on this/how frequent is the case in ITK. But it comes down to the derived class programmer the job of taking care of this: in the case at issue (SetNumberOfLevels) (regardless the method name, e.g. using an Initialize method), it seems to me that at times grouping related initializations in a method may be useful.

Thanks for your comment, Jon. But then, the derived class programmer just cannot customize the initialization during the construction of the base class by overriding a Initialize method from the base class. For the same reason as I mentioned at #196 (comment) So I think I don't really understand your case. Do you possibly have a code example for which this guideline ("All member variables must be initialized when they are declared") should not be followed? (Or maybe, might not need to be followed?)

ITKSoftwareGuide/SoftwareGuide/Latex/Appendices/CodingStyleGuide.tex

Line 1568 in 351ec22

All member variables must be initialized when they are declared. For such

There may be exceptions to this guideline, but without actual (preferably compilable) code examples, I find it hard to reason about them.

[dzenanz]

@N-Dekker I just invited you to https://github.com/orgs/InsightSoftwareConsortium/teams/itksoftwareguide/members.

[jhlegarreta]

#196 (comment) Sorry for the delay, Niels. I am fine now that Dženan merged this. We can revisit this as needed.

Do you possibly have a code example for which this guideline ("All member variables must be initialized when they are declared") should not be followed? (Or maybe, might not need to be followed?)
There may be exceptions to this guideline, but without actual (preferably compilable) code examples, I find it hard to reason about them.

As said #196 (review), I support the in-class brace initialization, but (i) the SWG may be out-of-sync in some sections to embrace that need; (ii) I think InsightSoftwareConsortium/ITK#3845 and the multiple related comments made about the SetNumberOfLevels method and its contents illustrate the point; will try to push a commit/open a PR that may make it clearer.

Thanks for the patience.

[jhlegarreta]

(ii) I think InsightSoftwareConsortium/ITK#3845 and the multiple related comments made about the SetNumberOfLevels method and its contents illustrate the point; will try to push a commit/open a PR that may make it clearer.

@N-Dekker InsightSoftwareConsortium/ITK#3876. Note that it is still a draft, as the initialization in the constructor vs. set method were already different (need to check the result and ask Nick as well), but it illustrates the point. My response pace may be irregular in the coming days/few weeks.