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

[Brad King]

Thompson, David C wrote:
> How much work would it be to just fix the wrapper generators
> to properly handle default arguments? It seems like that's
> what we should be doing when this kind of thing comes up.
> The same issue (wrappers) came up regarding enums
> recently and I would much prefer the wrapped interfaces
> to get better than see the C++ interface suffer.

Actually, the wrappers are not the only reason.  If the method is
virtual, which it may be due to inheritance even if the virtual keyword
is not present, adding an argument will change its signature and break
the overriding behavior.  Generally, virtual methods and default
arguments should not mix, and much of VTK's interface uses virtual
methods.  Instead of having a bunch of complicated rules to decide
whether or not a default argument is safe, it's easier to just say no
default arguments should be used.

-Brad

> ________________________________________
> From: vtk-developers-bounces at vtk.org [vtk-developers-bounces at vtk.org] On Behalf Of David Cole [david.cole at kitware.com]
> Sent: Wednesday, August 06, 2008 07:06
> To: VTK Developers
> Subject: [vtk-developers] Adding parameters to existing methods: do not use default argument values in C++ for revising VTK
> 
> 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.
> 
> 
> _______________________________________________
> vtk-developers mailing list
> vtk-developers at vtk.org
> http://www.vtk.org/mailman/listinfo/vtk-developers