[vtk-developers] RFC: vtkScopedPointer - a new hope?

[Marcus D. Hanwell]

On Tue, Jan 25, 2011 at 12:24 PM, Marcus D. Hanwell
<marcus.hanwell at kitware.com> wrote:
> On Tue, Jan 25, 2011 at 10:03 AM, Marcus D. Hanwell
> <marcus.hanwell at kitware.com> wrote:
>> On Tue, Jan 25, 2011 at 9:23 AM, Marcus D. Hanwell
>> <marcus.hanwell at kitware.com> wrote:
>>> On Mon, Jan 24, 2011 at 9:29 PM, David Cole <david.cole at kitware.com> wrote:
>>>> On Mon, Jan 24, 2011 at 8:21 PM, Marcus D. Hanwell
>>>> <marcus.hanwell at kitware.com> wrote:
>>>>> On Mon, Jan 24, 2011 at 7:05 PM, David Gobbi <david.gobbi at gmail.com> wrote:
>>>>>> On Mon, Jan 24, 2011 at 2:07 PM, David Cole <david.cole at kitware.com> wrote:
>>>>>>> On Mon, Jan 24, 2011 at 4:06 PM, David Cole <david.cole at kitware.com> wrote:
>>>>>>>> On Mon, Jan 24, 2011 at 3:48 PM, Marcus D. Hanwell
>>>>>>>> <marcus.hanwell at kitware.com> wrote:
>>>>>>>>> On Mon, Jan 24, 2011 at 3:41 PM, tom fogal <tfogal at sci.utah.edu> wrote:
>>>>>>>>>> "Marcus D. Hanwell" <marcus.hanwell at kitware.com> writes:
>>>>>>>>>>> Some of you may remember my dreams of a concise way of initializing
>>>>>>>>>>> VTK objects, that would go out of scope and be destroyed at the
>>>>>>>>>>> appropriate time. Inspired by the Qt scoped pointer, [. . .]
>>>>>>>>>>>
>>>>>>>>>>> http://review.source.kitware.com/759
>>>>>>>>>>
>>>>>>>>>> scoped (and other) smart pointers were introduced with tr1, back in
>>>>>>>>>> 2003 IIRC.  Further boost has had this for quite some time:
>>>>>>>>>>
>>>>>>>>>>  http://www.boost.org/doc/libs/1_45_0/libs/smart_ptr/scoped_ptr.htm?sess=94fbb4fa5cfd2b2071c3be193acb355d
>>>>>>>>>>
>>>>>>>>>> I realize that vtk is a bit stuck because it has already shipped some
>>>>>>>>>> smart pointers, and thus probably must continue to do so for some time
>>>>>>>>>> due to backward compat requirements, but I'd discourage *extending*
>>>>>>>>>> the trend.  This is a painful interop problem, especially with larger
>>>>>>>>>> software systems -- there's probably some C++ platitude that states,
>>>>>>>>>> "Every library eventually evolves a smart pointer implementation."
>>>>>>>>>>
>>>>>>>>>> Or there is now, at least.
>>>>>>>>>>
>>>>>>>>> Would any of these scoped pointers actually work with VTK derived
>>>>>>>>> objects? We have private constructors, need to call the static New
>>>>>>>>> method, and the Delete method on destruction. I also don't see VTK
>>>>>>>>> depending on TR1 features, or Boost for quite some time. The class is
>>>>>>>>> quite minimal, and I think it would complement the other two pointer
>>>>>>>>> classes that help to manage VTK derived objects.
>>>>>>>>>
>>>>>>>>
>>>>>>>> The boost discussion points out an important consideration here,
>>>>>>>> though: the name "scoped pointer" is assumed to be non-copyable and
>>>>>>>> non-transferrable out of the scoped context in the boost world. Here,
>>>>>>>> with your implementation, and with a reference counted system like
>>>>>>>> vtkObject, the ownership *can* be transferred outside the context of
>>>>>>>> the scoped pointer variable itself.
>>>>>>>>
>>>>>>>> So.... by using the same name as boost for something that is slightly
>>>>>>>> different, (but still extremely useful within VTK), we may be adding
>>>>>>>> to the heap of confusion that is smart/auto/scoped pointer discussions
>>>>>>>> in the C++ world.
>>>>>>>>
>>>>>>>> Might we be able to come up with a better name for it that does not
>>>>>>>> already have a different shade of meaning in the context of a "very
>>>>>>>> large" and "familiar to many" project like boost?
>>>>>>>>
>>>>>>>
>>>>>>> It's really just a vtkSmartPointer with an "easy create" syntax... Isn't it?
>>>>>>>
>>>>>>> Maybe vtkEasyPointer would be a better name?
>>>>>>
>>>>>> I like ScopedPointer better than EasyPointer.  But I liked the old
>>>>>> name vtkNew even more, because it made it obvious that an object was
>>>>>> being created.  How about another suggestion:
>>>>>>
>>>>>> vtkAutoPointer<vtkSomething> a;
>>>>>>
>>>>>> The name "auto" gives a sense that an object is being allocated.  It
>>>>>> implies "this creates an automatically garbage-collected object",
>>>>>> while the name vtkScopedPointer makes it sound like a pointer is being
>>>>>> created but not an object.
>>>>>
>>>>> I wonder if vtkScopedNew might satisfy more people? Brad was
>>>>> explaining the VTK convention that anything that instantiates should
>>>>> have New in its name, but he would not like to use vtkNew. It is a
>>>>> scoped new object, and so I feel like it could be a reasonable
>>>>> compromise clearing up any ambiguity.
>>>>>
>>>>> So in a test, one can simply use,
>>>>>
>>>>> vtkScopedNew<vtkClass> a;
>>>>> a->DoSomething();
>>>>>
>>>>> When it goes out of scope, reference count is decremented. If you want
>>>>> the raw pointer you must call GetPointer, and can use something like,
>>>>>
>>>>> vtkSmartPointer<vtkClass> b = a.GetPointer();
>>>>>
>>>>> I think this is a secondary use case of this class, and if you simply
>>>>> want a smart pointer with an object, you should stick with the
>>>>> conventional syntax. The primary use case was to replace the VTK
>>>>> create macro used in many tests. It should also serve the use case of
>>>>> ivars in classes where you cannot set that object (as you cannot
>>>>> assign to a vtkScoped[TBD] object.
>>>>>
>>>>> I did like vtkNew, but the more I think about it vtkScopedNew seems to
>>>>> encapsulate its name, and accomplishes one of the primary goals when
>>>>> this thread was started all that time ago.
>>>>>
>>>>> Marcus
>>>>>
>>>>
>>>>
>>>> I like just "vtkNew" the best.
>>>>
>>>> Neither Auto nor Scoped mean quite the same thing here as in other contexts.
>>>>
>>>> Why don't we want to use vtkNew in this situation? What are we saving it for?
>>>>
>>>> Or, we could use "vtkNu" -- it's shorter, and it's sort of a Greek letter. ;-)
>>>>
>>> vtkNu sounds good, and saves a character ;-) Main reason was Brad
>>> saying he wanted to reserve vtkNew for some future use, maybe he could
>>> explain that better than I could. I was always happy with vtkNew.
>>>
>>> I think the assignment operator interaction with vtkSmartPointer from
>>> the original is not necessary, and adds more complication/confusion to
>>> the API. I think this was our point of divergence when we were
>>> discussing this offline.
>>>
>> I have updated the proposed change, now called vtkNew. Not to sway any
>> of you, but it is my birthday tomorrow and I have wanted something
>> like this in VTK for so long now ;-)
>>
>> http://review.source.kitware.com/#change,759
>>
> Also,
>
> http://review.source.kitware.com/#change,768
>
> Added a class to the test, demonstrating the use of an incomplete type
> in a classes ivars, assignment to a smart pointer etc. This is another
> compelling place where we can remove a lot of boiler plate code in the
> constructors of classes.
>
> Potential users of this should note that you cannot set the pointer a
> vtkNew contains, and vtkSmartPointer or vtkWeakPointer would remain
> better choices there depending upon your requirements.
>

For the benefit of the list, Bill Lorensen pointed out,

>I just tried changing a widgets test to use vtkNew.
> I had hoped I could just replace vtkSmartPointer with vtkNew.
>I tried:
> vtkNew<vtkRenderer
> ren1;
> vtkNew<vtkRenderWindow> renWin;
> renWin->AddRenderer(ren1);
> vtkNew<vtkRenderWindowInteractor> iren;
> iren->SetRenderWindow(renWin);
>
>But needed:
> vtkNew<vtkRenderer> ren1;
> vtkNew<vtkRenderWindow> renWin;
> renWin->AddRenderer(ren1.GetPointer());
> vtkNew<vtkRenderWindowInteractor> iren;
> iren->SetRenderWindow(renWin.GetPointer());
>
>I recall that Brad's vtkNew effort a while back was much cleaner. I can't recall the pros and cons of his effort.

This is intended behavior. If we had an implicit cast operator (the
thing you are lacking here), then a developer could write,
vtkClass * doSomething(int val)
{
  vtkNew<vtkClass> object;
  object->SetValue(5);
  object->DoIt();
  ...
  return object;
}

When calling this function, the returned pointer would be invalid. By
only giving access to the pointer through a getter, the developer must
at least stop, realize this is not a normal pointer and either decide
to use the raw pointer, or change their approach.

When you call GetPointer() you are taking some extra responsibility on
to ensure the pointer remains valid. It would generally be the wrong
thing to do with a temporary variable, but for a class ivar it would
be acceptable to return that pointer as it remains valid for the life
of the class instance.

I think it is important to omit the implicit cast operator for this
reason, even if it does lead to a little extra code. It would cause
bad code to fail to compile, rather than introducing bugs through the
accidental return of the pointer the vtkNew object holds a reference
to. I think this was part of a discussion I had with Brad, but I had
meant to point it out on the list too.

Marcus