[vtk-developers] vtkVector class -- proposed rewrite

[David Gobbi]

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.

Using typedefs might simplify the implementation, but they make
the class just a bit more difficult to use.  I would strongly prefer
being able to do a "class vtkVector3f" forward declaration instead
of including a header file.

 - David