[vtk-developers] vtkVector class -- proposed rewrite

[Marcus D. Hanwell]

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.
>
> 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).
>
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.

Marcus