[vtk-developers] vtkVector class -- proposed rewrite

[David Gobbi]

On Mon, Aug 8, 2011 at 1:58 PM, David Lonie <loniedavid at gmail.com> wrote:
> On Tue, Aug 2, 2011 at 4:20 PM, David Lonie <loniedavid at gmail.com> wrote:
>> On Tue, Aug 2, 2011 at 3:56 PM, Marcus D. Hanwell
>> <marcus.hanwell at kitware.com> wrote:
>>> On Tue, Aug 2, 2011 at 11:29 AM, David Gobbi <david.gobbi at gmail.com> wrote:
>>>> On Tue, Aug 2, 2011 at 8:13 AM, David Lonie <loniedavid at gmail.com> wrote:
>>>>> On Mon, Jul 25, 2011 at 3:22 PM, David Gobbi <david.gobbi at gmail.com> wrote:
>>>>>> On Mon, Jul 25, 2011 at 12:17 PM, David Lonie <loniedavid at gmail.com> wrote:
>>>>>>>
>>>>>>> Would using a typedef be possible for the wrappers? e.g. "typedef
>>>>>>> vtkVector<float, 3> vtkVector3f"? That would eliminate most of the
>>>>>>> complexity in this patch.
>>>>>>
>>>>>> Right now, no, it won't work, but it would be easy to make it work,
>>>>>> easier than handling the macro expansion
>>>>>
>>>>> Could this be added soon? I hope to submit the vtkChemistry patches to
>>>>> gerrit soon, and this is a prerequisite. I understand if it will be a
>>>>> while before this is added, I just want to know if I need to start
>>>>> thinking about other approaches to the vector class in the meantime.
>>>>
>>>> Finalize your design for vtkVector first, and then I'll see what I can
>>>> do to get it wrapped.  Both the macro approach and the typedef
>>>> approach are potentially wrappable.  I cannot give a specific timeline
>>>> for either of them.  Like most of my voluntary contributions to VTK,
>>>> unless there is a specific incentive, it will happen when I feel like
>>>> doing it.
>>>>
>>> If there is no preferred approach from a wrapping point of view then I
>>> would suggest we pursue the typedef appraoch as I think that will give
>>> us the most maintainable code in the long run.
>>
>> I agree -- I'll have a new patch ready sometime this week.
>
> I've pushed a new branch, vtkVector-typedef, to my github repo:
>
> http://github.com/dlonie/VTK/commits/vtkVector-typedef
>
> This adds:
>
> vtkVectorBase<typename T, int Size>
> vtkVector<typename T, int Size>
> typedefs for common vectors (3f, 4d, 2i, etc)
>
> I've also added vtkVectorForwardDeclarations.h for headers that use
> the typedefs but don't need the entire implementation.
>
> If there are objections to this approach, or if there are changes that
> will make it easier to parse, please let me know.

Check the VTK style guidelines:

http://www.vtk.org/Wiki/VTK_Coding_Standards

Code such as the following does not fit the guidelines:

  bool operator==(const vtkVectorBase<T, Size> &other)
  {
    for (int i = 0; i < Size; ++i)
      if (this->Data[i] != other.Data[i])
        return false;
    return true;
  }

Also, the use of the vtkVectorForwardDeclarations header is used is
ugly. Now that I see the typedefs in action, it is apparent that they
are not a very good fit for VTK because you cannot do a forward
declaration of a typedef, and forward declarations are very much a
part of good coding style.

Unfortunately removing the typedefs requires defining subclasses with
all the necessary constructors, and I know you wanted to avoid that.

Over all, I preferred your earlier macro-based solution to these
typedefs.  Even though the typedef solution might be easier to wrap,
it seems to be a poorer solution from the C++ side.  (From my
perspective the ideal solution would be 100% explicit, with all the
code written out in full, but I understand your concerns about
maintaining such a monster).

 - David