[vtk-developers] vtkVector class -- proposed rewrite

[David Lonie]

On Thu, Aug 11, 2011 at 7:46 AM, David Gobbi <david.gobbi at gmail.com> wrote:
> On Thu, Aug 11, 2011 at 3:45 AM, Marcus D. Hanwell
> <marcus.hanwell at kitware.com> wrote:
>> On Wed, Aug 10, 2011 at 11:21 AM, David Gobbi <david.gobbi at gmail.com> wrote:
>>> 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.
>>
>> You can do a forward declaration, it is two lines instead of one. The
>> forward declaration header is there as a convenience as far as I can
>> tell.
>>
>> It seems a shame to avoid the use of typedefs here, as far as I can
>> see you can forward declare but an extra line forward declaring the
>> template is required before the line declaring the typedef. If it can
>> be wrapped I don't think I see the problem on the C++ side.
>
> I don't agree that the two-line forward declaration is a true forward
> declaration.  Case in point: it makes the new vtkVector3f etc.
> backwards-incompatible with VTK 5.6 and VTK 5.8.  In the commit
> diff that David linked to, I can see that many other VTK classes
> had to be changed because of the switch to typedefs.

I put the macro'd implementation up here after rearranging it a bit
and fixing the style:
http://github.com/dlonie/VTK/tree/macro-vtkVector

The typedef code has been moved to here:
http://github.com/dlonie/VTK/tree/typedef-vtkVector

The backwards compatibility point is a good one, I think it settles
the discussion. I plan to submit the macro'd version to gerrit in the
next day or so if there are no other issues with it.

Dave