[vtk-developers] Adding parameters to existing methods: do not use default argument values in C++ for revising VTK

[David Cole]

I know this is a bit on the long side. Please read when you have a chance,
especially if you are an active VTK contributor.

I have had this same conversation with a few different people over the last
few months and want to share with the VTK dev community just so that we are
all on the same page. And to give you all an opportunity to voice your
opinions on the matter. Hmmm... maybe this should be in our coding
guidelines on the Wiki.
When adding functionality in C++ it is tempting to add new parameters to
existing methods and give default values to the new parameters. This is a
snap from C++ and works great: all dependent clients simply recompile
against the new header and everything just works. Now you can have clients
that call the method with the old number of parameters or with the new
additional parameters, too.

However, this doesn't work so good from wrapped languages. If there is a
method (just a hypothetical example) like this:
  vtkIdType AddChild(vtkIdType parent);

...and you change it to this:
  vtkIdType AddChild(vtkIdType parent,
                     vtkVariantArray *propertyArr = 0);

...suddenly from the tcl wrapped version you can no longer do this:
  vtkMutableDirectedGraph g
  g AddChild 0

...you'll get a runtime error from the tcl interpreter saying: "there is no
AddChild that takes 1 parameter" (paraphrased...)

Worse still: there is no indication from the VTK dashboard that anything has
gone wrong unless there is a tcl test (or perhaps python or java) that
executes that method.

Rather, the "proper" way (or at least the "wrapped languages safe" way) to
accomplish your goal is to add an overload without any default parameters
and then call the new overload from the old signature's implementation. Like
this:
  // 2 signatures in the header file, no default parameter values: default
values can be unintentionally evil
  vtkIdType AddChild(vtkIdType parent);
  vtkIdType AddChild(vtkIdType parent,
                     vtkVariantArray *propertyArr);

  // and in the cxx file:
  vtkIdType vtkMutableDirectedGraph::AddChild(vtkIdType parent)
  {
    return this->AddChild(parent, 0);
  }

  vtkIdType vtkMutableDirectedGraph::AddChild(vtkIdType parent,
                     vtkVariantArray *propertyArr)
  {
    // old implementation of the original method that takes the new
parameter
    // value into account....
  }


Another related lesson along these lines:
There has been a tendency for many of us to write the tests for our new code
in C++ to maximize the number of dashboards/platforms on which a test runs.
However, I would strongly encourage you all to think about submitting tcl
tests as well for just this reason. It's a better test of the API
compatibility and will indicate when unintentional API breakages occur on
the dashboards that do turn on tcl wrapping.


Any comments, thoughts, objections, agreement....?


Thanks,
David Cole
Kitware, Inc.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://public.kitware.com/pipermail/vtk-developers/attachments/20080806/9de4b183/attachment.html>