[vtk-developers] Smart pointer declaration macro?

Thu Jan 28 09:30:25 EST 2010

On Thu, Jan 28, 2010 at 9:24 AM, Wes Turner <wes.turner at kitware.com> wrote:
> I'm not really against the idea, but I am not convinced that this actually
> does anything real for us.  We are just replacing the line:
> vtkSmartPointer<vtkClass> myLocal = vtkSmartPointer<vtkClass>::New();
> with
> vtkSmartPointer<vtkClass> myLocal(true);
> I don't mind change and will go along with whatever is decided, but we are
> trading conciseness for clarity.  I generally don't mind multiple line code,
> so shortening the signature is not a big deal to me.  Is there really
> consensus that this is needed?

I think people will either use this or else some ad hoc macros like
the VTK_CREATE macro that's currently scattered around tests/examples.
Since I've seen several people use a macro (myself included), I think
that shows some sort of consensus that the current length of code
required for initializing smart pointers is unacceptable.

Jeff

> On Thu, Jan 28, 2010 at 9:05 AM, David Doria <daviddoria+vtk at gmail.com>
> wrote:
>>
>> On Thu, Jan 28, 2010 at 9:01 AM, Will Schroeder
>> <will.schroeder at kitware.com> wrote:
>> > This has been part of the VTK naming convention as well, at least in
>> > practice.
>> >
>> > On Thu, Jan 28, 2010 at 8:54 AM, Karthik Krishnan
>> > <karthik.krishnan at kitware.com> wrote:
>> >>
>> >> Abbreviations in ITK are in the "things to avoid list" as far as
>> >> naming conventions go, unless the abbreviation is a standard one used
>> >> in the field such as FFT ..
>> >>
>> >> I suppose this is part of the VTK coding convention as well ? If not
>> >> it should probably be in there
>> >>
>> >> On Thu, Jan 28, 2010 at 8:50 AM, Will Schroeder
>> >> <will.schroeder at kitware.com> wrote:
>> >> > Sorry, I don't like the abbreviation, I love change :-)
>> >> >
>> >> >
>> >> > On Thu, Jan 28, 2010 at 8:45 AM, Jeff Baumes
>> >> > <jeff.baumes at kitware.com>
>> >> > wrote:
>> >> >>
>> >> >> On Wed, Jan 27, 2010 at 10:58 AM, Will Schroeder
>> >> >> <will.schroeder at kitware.com> wrote:
>> >> >> > VTK has a verbose, self documenting style (for better or worse).
>> >> >> > I'd
>> >> >> > like to
>> >> >> > stick with it if possible.
>> >> >>
>> >> >> Will,
>> >> >>
>> >> >> Are you suggesting no change? Or just that you don't like the
>> >> >> abbreviated typedefs like vtkRendererSP?
>> >> >>
>> >> >> Jeff
>> >> >>
>> >> >> >
>> >> >> >
>> >> >> > On Wed, Jan 27, 2010 at 10:56 AM, David Cole
>> >> >> > <david.cole at kitware.com>
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> On Wed, Jan 27, 2010 at 8:35 AM, Jeff Baumes
>> >> >> >> <jeff.baumes at kitware.com>
>> >> >> >> wrote:
>> >> >> >>>
>> >> >> >>> On Tue, Jan 26, 2010 at 12:44 PM, Marcus D. Hanwell
>> >> >> >>> <marcus.hanwell at kitware.com> wrote:
>> >> >> >>> > On Friday 08 January 2010 16:46:47 David Cole wrote:
>> >> >> >>> >> On Fri, Jan 8, 2010 at 4:08 PM, Marcus D. Hanwell <
>> >> >> >>> >>
>> >> >> >>> >> marcus.hanwell at kitware.com> wrote:
>> >> >> >>> >> > I would like to add my -1, I think needing to split lines
>> >> >> >>> >> > in
>> >> >> >>> >> > order
>> >> >> >>> >> > to
>> >> >> >>> >> > declare
>> >> >> >>> >> > a new local variable is a little much. I came from a C++
>> >> >> >>> >> > background
>> >> >> >>> >> > where
>> >> >> >>> >> > any
>> >> >> >>> >> > object could be declared on the stack though. For things
>> >> >> >>> >> > like
>> >> >> >>> >> > the
>> >> >> >>> >> > examples it
>> >> >> >>> >> > seems to hurt readability to me.
>> >> >> >>> >> >
>> >> >> >>> >> > Pointer:
>> >> >> >>> >> > vtkFloatArray *myTable = vtkFloatArray::New();
>> >> >> >>> >> > myTable->Delete();
>> >> >> >>> >> > myTable = NULL;
>> >> >> >>> >> >
>> >> >> >>> >> > Smart pointer:
>> >> >> >>> >> > vtkSmartPointer<vtkFloatArray> myTable =
>> >> >> >>> >> >     vtkSmartPointer<vtkFloatArray>::New();
>> >> >> >>> >> >
>> >> >> >>> >> > Smart pointer with macro:
>> >> >> >>> >> > VTK_CREATE(vtkFloatArray, myTable);
>> >> >> >>> >> >
>> >> >> >>> >> > Stack:
>> >> >> >>> >> > vtkFloatArray myTable;
>> >> >> >>> >> >
>> >> >> >>> >> > I would prefer to be able to use something like the first
>> >> >> >>> >> > or
>> >> >> >>> >> > the
>> >> >> >>> >> > last. In
>> >> >> >>> >> > classes etc it is often a different story. It seems like
>> >> >> >>> >> > there
>> >> >> >>> >> > should be
>> >> >> >>> >> > some
>> >> >> >>> >> > macro or template function to generate variables with less
>> >> >> >>> >> > repetition.
>> >> >> >>> >>
>> >> >> >>> >> Prefer the first and last as much as you want, but we simply
>> >> >> >>> >> can't
>> >> >> >>> >> use
>> >> >> >>> >> them
>> >> >> >>> >> in VTK. The first leads to memory leaks because people forget
>> >> >> >>> >> the
>> >> >> >>> >> Delete
>> >> >> >>> >> calls. The last cannot be done with vtkObject derived classes
>> >> >> >>> >> because
>> >> >> >>> >> of
>> >> >> >>> >>  the nature of vtkObject reference counting...
>> >> >> >>> >>
>> >> >> >>> > Wasn't suggesting either (just pointing out the shorter syntax
>> >> >> >>> > that
>> >> >> >>> > people
>> >> >> >>> > miss), although the first is still widely used in VTK.
>> >> >> >>> >
>> >> >> >>> >> So we have to pick one of the middle ones...
>> >> >> >>> >>
>> >> >> >>> >> It's unfortunate that we've had two +1's and two -1's... that
>> >> >> >>> >> leaves
>> >> >> >>> >> us at
>> >> >> >>> >>  0 for the moment. I guess that and the fact that it's Friday
>> >> >> >>> >> makes
>> >> >> >>> >> it
>> >> >> >>> >>  fairly easy to put off a decision until at least next week.
>> >> >> >>> >> ;-)
>> >> >> >>> >>
>> >> >> >>> >> *If* we do go with a macro-based approach, I think we can all
>> >> >> >>> >> agree
>> >> >> >>> >> there
>> >> >> >>> >> should be one centralized macro that does this and it should
>> >> >> >>> >> be
>> >> >> >>> >> used
>> >> >> >>> >> *everywhere* that vtkSmartPointer::New is presently used.
>> >> >> >>> >
>> >> >> >>> > What about a vtkLocalPointer<vtkClass> myLocal; where the
>> >> >> >>> > default
>> >> >> >>> > constructor
>> >> >> >>> > makes an instance on the VTK class? It would also be possible
>> >> >> >>> > to
>> >> >> >>> > have a
>> >> >> >>> > constructor take an argument (may be a little clunkier) such
>> >> >> >>> > as
>> >> >> >>> > vtkSmartPointer<vtkClass> myLocal(true); if we do not want to
>> >> >> >>> > introduce
>> >> >> >>> > yet
>> >> >> >>> > another class.
>> >> >> >>> >
>> >> >> >>> > Alternate options a and b...
>> >> >> >>> >
>> >> >> >>> > a) vtkLocalPointer<vtkClass> myLocal;
>> >> >> >>> > b) vtkSmartPointer<vtkClass> myLocal(true);
>> >> >> >>> >
>> >> >> >>> > Would this be preferable to a macro? It seems like a better
>> >> >> >>> > way
>> >> >> >>> > to
>> >> >> >>> > go
>> >> >> >>> > to me,
>> >> >> >>> > and in terms of API and conciseness seems to satisfy our
>> >> >> >>> > requirements
>> >> >> >>> > better
>> >> >> >>> > than the current approach. It would still be possible to share
>> >> >> >>> > the
>> >> >> >>> > pointer
>> >> >> >>> > too, due to the reference counting in vtkObject derived
>> >> >> >>> > classes.
>> >> >> >>>
>> >> >> >>> I think option b is a good option. It seems that it would be odd
>> >> >> >>> to
>> >> >> >>> have two different pointer classes, where the only difference is
>> >> >> >>> what
>> >> >> >>> they do on construction.
>> >> >> >>>
>> >> >> >>> The biggest grudge I have about the current option is that it
>> >> >> >>> almost
>> >> >> >>> always requires a split line if you constrain yourself to 80
>> >> >> >>> characters. If your type name is more than about 15 characters
>> >> >> >>> (which
>> >> >> >>> many VTK types are), you have to split your line:
>> >> >> >>>
>> >> >> >>> vtkSmartPointer<type> name = vtkSmartPointer<type>::New();
>> >> >> >>> 16 + t + 2 + n + 19 + t + 9 = 46 + 2*t + n
>> >> >> >>>
>> >> >> >>> Option b, even though not the most concise, would make it easy
>> >> >> >>> to
>> >> >> >>> stay
>> >> >> >>> within that limit, about halving the number of characters:
>> >> >> >>>
>> >> >> >>> vtkSmartPointer<type> name(true);
>> >> >> >>> 16 + t + 2 + n + 7 = 25 + t + n
>> >> >> >>>
>> >> >> >>> Jeff
>> >> >> >>
>> >> >> >>
>> >> >> >> Another solution worth considering is simply making a typedef
>> >> >> >> called
>> >> >> >> "vtkRendererSP" that is a typedef for
>> >> >> >> "vtkSmartPointer<vtkRenderer>"
>> >> >> >> --
>> >> >> >> that
>> >> >> >> would allow you to:
>> >> >> >> vtkRendererSP renderer = vtkRendererSP::New();
>> >> >> >> It gives you the shorter names you're all longing for, without
>> >> >> >> changing
>> >> >> >> anything else already in VTK...
>> >> >> >>
>> >> >> >> David C.
>> >> >> >>
>> >> >> >> _______________________________________________
>> >> >> >> Powered by www.kitware.com
>> >> >> >>
>> >> >> >> Visit other Kitware open-source projects at
>> >> >> >> http://www.kitware.com/opensource/opensource.html
>> >> >> >>
>> >> >> >> Follow this link to subscribe/unsubscribe:
>> >> >> >> http://www.vtk.org/mailman/listinfo/vtk-developers
>> >> >> >>
>> >> >> >>
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >> > --
>> >> >> > William J. Schroeder, PhD
>> >> >> > Kitware, Inc.
>> >> >> > 28 Corporate Drive
>> >> >> > Clifton Park, NY 12065
>> >> >> > will.schroeder at kitware.com
>> >> >> > http://www.kitware.com
>> >> >> > (518) 881-4902
>> >> >> >
>> >> >>
>> >> >>
>> >> >>
>> >> >> --
>> >> >> Jeff Baumes, Ph.D.
>> >> >> R&D Engineer, Kitware Inc.
>> >> >> (518) 881-4932
>> >> >> jeff.baumes at kitware.com
>> >> >
>> >> >
>> >> >
>> >> > --
>> >> > William J. Schroeder, PhD
>> >> > Kitware, Inc.
>> >> > 28 Corporate Drive
>> >> > Clifton Park, NY 12065
>> >> > will.schroeder at kitware.com
>> >> > http://www.kitware.com
>> >> > (518) 881-4902
>> >> >
>> >> > _______________________________________________
>> >> > Powered by www.kitware.com
>> >> >
>> >> > Visit other Kitware open-source projects at
>> >> > http://www.kitware.com/opensource/opensource.html
>> >> >
>> >> > Follow this link to subscribe/unsubscribe:
>> >> > http://www.vtk.org/mailman/listinfo/vtk-developers
>> >> >
>> >> >
>> >> _______________________________________________
>> >> Powered by www.kitware.com
>> >>
>> >> Visit other Kitware open-source projects at
>> >> http://www.kitware.com/opensource/opensource.html
>> >>
>> >> Follow this link to subscribe/unsubscribe:
>> >> http://www.vtk.org/mailman/listinfo/vtk-developers
>> >>
>> >
>> >
>> >
>> > --
>> > William J. Schroeder, PhD
>> > Kitware, Inc.
>> > 28 Corporate Drive
>> > Clifton Park, NY 12065
>> > will.schroeder at kitware.com
>> > http://www.kitware.com
>> > (518) 881-4902
>>
>> OK, so is everyone on board with Marcus' suggestion of:
>>
>> vtkSmartPointer<vtkClass> myLocal(true);
>>
>> If so, who is going to do it?
>>
>> Thanks,
>>
>> David
>> _______________________________________________
>> Powered by www.kitware.com
>>
>> Visit other Kitware open-source projects at
>> http://www.kitware.com/opensource/opensource.html
>>
>> Follow this link to subscribe/unsubscribe:
>> http://www.vtk.org/mailman/listinfo/vtk-developers
>>
>
>
>
> --
> Wesley D. Turner, Ph.D.
> Kitware, Inc.
> Technical Leader
> 28 Corporate Drive
> Clifton Park, NY 12065-8662
> Phone: 518-881-4920
>
> _______________________________________________
> Powered by www.kitware.com
>
> Visit other Kitware open-source projects at
> http://www.kitware.com/opensource/opensource.html
>
> Follow this link to subscribe/unsubscribe:
> http://www.vtk.org/mailman/listinfo/vtk-developers
>
>
>