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

[Bill Lorensen]

Marcus,

I think I understand your argument.

However, I was looking for a simple drop-in similar to what Brad
experimented with last March.

I can't recall the details of his implementation nor why we abandoned it.

Bill

On Tue, Jan 25, 2011 at 1:00 PM, Marcus D. Hanwell
<marcus.hanwell at kitware.com> wrote:
> 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
>