[vtk-developers] [coding tip] no call to Set in constructors

[David Cole]

I will say that you should avoid "Set" methods entirely from constructors
unless you verify the following:-- the Set method in question is *not*
*virtual*

If the Set method is virtual and you call it from a constructor of a class
that's mid-way through a deeply nested hierarchy of classes (it has parent
classes and sub-classes defined) then you are trying to do something that is
probably not what you expect. A C++ object should only initialize its own
classes ivars (or possibly some from its parent classes if it has to give
different default values for some of those) in the constructor. Nothing
else. Anything else should be done after the construction of the object is
complete in a 2nd pass "initialize(with, these, params);" type of call.

You are inviting unexplained behavior to occur (eventually, for someone) if
you call virtual methods from the constructor.

It has always seemed to me that there should be a C++ compiler warning
issued when a virtual method is called from a constructor.


Other opinions welcome...
:-)
David Cole


On Wed, Apr 29, 2009 at 9:52 AM, Francois Bertel <
francois.bertel at kitware.com> wrote:

> Thanks for clarifying and pointing out the case of strings and objects.
>
> I actually said "cannot be used in the default constructor to
> *initialize ivars*". I haven't say you couldn't use them in the
> constructor for changing ivar values once they are initialized.
>
> On Wed, Apr 29, 2009 at 9:40 AM, Moreland, Kenneth <kmorel at sandia.gov>
> wrote:
> > A slight qualification: it is OK to call the Set function in the
> > constructors _so long as you initialize the underlying variable first_.
>  I
> > find I do this frequently with the string and object ivars to ensure that
> > the memory management happens correctly.  For example:
> >
> > this->FileName = NULL;
> > this->SetFileName("/tmp/log");
> >
> > or
> >
> > this->Controller = NULL;
> > this->SetController(vtkMultiProcessController::GetGlobalController());
> >
> > Of course, setting the ivar to NULL first is vital, or things could
> > potentially die a really horrible death.
> >
> > -Ken
> >
> >
> > On 4/29/09 7:31 AM, "Francois Bertel" <francois.bertel at kitware.com>
> wrote:
> >
> > Hello,
> >
> > I've seen that more often in recent additions to VTK: calls to some
> > Set methods in the default constructor.
> >
> > I would like to stress that Set methods defined with vtkSetMacro (it
> > might be OK for other Set methods) cannot be used in the default
> > constructor to initialize ivars because the first line of a Set method
> > is to compare the current value of the ivar with the value in
> > argument. As at this point (in the constructor), the ivar is not
> > initialized yet, the comparison happens against an uninitialized
> > value.
> >
> > valgrind will detect this kind of fault.
> >
> > For this reason, in the constructor, the value of an ivar has to be
> > initialized directly with the assignment operator not through a Set
> > method defined with vtkSetMacro.
> >
> > Example:
> >
> > class vtkFoo : public vtkObject
> > {
> >  public:
> > ...
> >   vtkGetMacro(X,int);
> >   vtkSetMacro(X,int);
> > ...
> >  protected:
> >   vtkFoo();
> >   int X;
> > ...
> > };
> >
> > vtkFoo::vtkFoo
> > {
> >  this->SetX(12); // neh
> > }
> >
> > vtkFoo::vtkFoo
> > {
> >  this->X=12; // good
> > }
> >
> > The issue looks pretty obvious in an isolated example like the one
> > shown above. Sometimes it can also happen with an indirect call:
> >
> > void vtkFoo::SetXToZero()
> > {
> >  this->SetX(0);
> > }
> >
> > vtkFoo::vtkFoo
> > {
> >  this->SetXToZero(); // neh
> > }
> >
> > I added a section addressing this issue in the VTK Coding Standards:
> > http://www.vtk.org/Wiki/VTK_Coding_Standards#Set_method_in_constructor
> >
> >
> >
> > Here is a real case in VTK, detected by valgrind:
> >
> > http://www.cdash.org/CDash/viewDynamicAnalysisFile.php?id=325444
> >
> > vtkRenderView::vtkRenderView() calls SetInteractionModeTo3D(), which
> > calls SetInteractionMode(), but ivar InteractionMode has not been
> > initialized yet.
> >
> >
> >
> > --
> > François Bertel, PhD  | Kitware Inc. Suite 204
> > 1 (518) 371 3971 x113 | 28 Corporate Drive
> >                       | Clifton Park NY 12065, USA
> > _______________________________________________
> > Powered by www.kitware.com
> >
> > Visit other Kitware open-source projects at
> > http://www.kitware.com/opensource/opensource.html
> >
> > Follow this link to subscribe/unsubscribe:
> > http://www.vtk.org/mailman/listinfo/vtk-developers
> >
> >
> >
> >
> >
> >    ****      Kenneth Moreland
> >     ***      Sandia National Laboratories
> > ***********
> > *** *** ***  email: kmorel at sandia.gov
> > **  ***  **  phone: (505) 844-8919
> >     ***      web:   http://www.cs.unm.edu/~kmorel
> >
> >
>
>
>
> --
> François Bertel, PhD  | Kitware Inc. Suite 204
> 1 (518) 371 3971 x113 | 28 Corporate Drive
>                      | Clifton Park NY 12065, USA
> _______________________________________________
> Powered by www.kitware.com
>
> Visit other Kitware open-source projects at
> http://www.kitware.com/opensource/opensource.html
>
> Follow this link to subscribe/unsubscribe:
> http://www.vtk.org/mailman/listinfo/vtk-developers
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://public.kitware.com/pipermail/vtk-developers/attachments/20090429/a40f366a/attachment.html>