[Insight-developers] Returning a smart pointer or a pointer?

[Tom Vercauteren]

Hi Luis,

Thanks for the changes!

As far as encouraging people to use the raw pointer function, I don't
see a better solution than documenting it as you did.

Tom

On Mon, May 19, 2008 at 11:39 PM, Luis Ibanez <luis.ibanez at kitware.com> wrote:
>
> Bill, Tom,
>
>
> The method has been added to itkPDEDeformableRegistrationFunction:
> http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Algorithms/itkPDEDeformableRegistrationFunction.h?root=Insight&r1=1.17&r2=1.18&sortby=date
>
>
> It is called:
>
>
>          GetDeformationFieldRawPointer()
>
>
> by (temporarily) commenting out the GetDeformationField() method,
> its use was identified (and replaced) in the following classes:
>
>
> itkSymmetricForcesDemonsRegistrationFunction.txx
> http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Algorithms/itkSymmetricForcesDemonsRegistrationFunction.txx?root=Insight&r1=1.4&r2=1.5&sortby=date
>
> itkMeanSquareRegistrationFunction.txx
> http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Algorithms/itkMeanSquareRegistrationFunction.txx?root=Insight&r1=1.13&r2=1.14&sortby=date
>
> itkNCCRegistrationFunction.txx
> http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Algorithms/itkNCCRegistrationFunction.txx?root=Insight&r1=1.11&r2=1.12&sortby=date
>
> itkMIRegistrationFunction.txx
> http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Algorithms/itkMIRegistrationFunction.txx?root=Insight&r1=1.17&r2=1.18&sortby=date
>
>
> We should be able to see the difference in tomorrow's
> code coverage, and in the timing of the Demons' test.
>
>
> This should have preserved Backward Compatibility.
>
>
> The open question is:
>
>
>    How to discurage developers from using GetDeformationField()
>    in the future ?
>
>
>
>
>    Luis
>
>
> -------------------
> Bill Lorensen wrote:
>>
>> If we do add a new method, I think it should have a RawPointer suffix.
>> Pointer is used as a suffix to typedefs of smart pointers.
>>
>> Also, I just looked in the SymmetricForcesDemonsRegistration class and
>> it calls the GetDeformationField method frequently. Also, in the
>> coverage PDEDeformableRegistrationFuntion::GetDefromationField() is
>> called 79,000,000 times.
>>
>>
>> http://www.itk.org/Testing/Sites/zion.kitware/Linux-g++-3.4/Coverage/__Code_Algorithms_itkPDEDeformableRegistrationFunction_h.html
>>
>> Bill
>>
>> On Mon, May 19, 2008 at 1:35 PM, Luis Ibanez <luis.ibanez at kitware.com>
>> wrote:
>>
>>> Hi Tom, Bill, Ken,
>>>
>>> Here is a suggestion:
>>>
>>>
>>> 1) Add a new method called
>>>
>>>
>>>    FieldType * GetDeformationFieldPointer()
>>>
>>>  or
>>>
>>>    FieldType * GetDeformationFieldRawPointer()
>>>
>>> Then track from where in ITK the original method
>>> was being called, and replace it with this new
>>> one.
>>>
>>>
>>> 2) This may also require to do the same function
>>>  addition in the base classes of the PDFDeformable
>>>  RegistrationFunction, and its derived classes,
>>>  as well as the invocation of that method from
>>>  all the classes.
>>>
>>> This will keep the original method around, and still
>>> should help to solve the performance issue inside ITK.
>>>
>>> Additionally, we should add a comment in the GetDeformationField()
>>> method, warning users that the use of this method involves a
>>> performance penalty and the an alternative, faster method,
>>> is available.
>>>
>>>
>>>  Luis
>>>
>>>
>>> -------------------------
>>> Tom Vercauteren wrote:
>>>
>>>> Hi all,
>>>>
>>>>
>>>>
>>>>> The change will affect users that create a subclass and override the
>>>>> method.
>>>>
>>>> This was indeed the backwards compatibility issue I was thinking of. I
>>>> just cannot really figure why someone would do something like that...
>>>> But as Bill said, this is unpredictable.
>>>>
>>>>
>>>>
>>>>
>>>>>> a) When the object to be returned is a member variable of the
>>>>>> class, then a raw pointer is returned, and preferably a
>>>>>> "const" raw pointer.
>>>>>>
>>>>>>
>>>>>> b) When the object to be returned is a newly created object,
>>>>>> then a SmartPointer is the preferred method, since it will
>>>>>> force users of this class to receive the value in a SmartPointer
>>>>>> and will prevent the returned object from "death in transit".
>>>>
>>>> Ok thanks for the clarification.
>>>>
>>>>
>>>>
>>>>> How much of a performance hit are you seeing?
>>>>>
>>>>> Maybe we will have to define a new method? Are their other classes
>>>>> that have the same p[problem?
>>>>
>>>> I don' remember the exact performance hit. I was at least a few
>>>> percents since it was appearing on the profiler output. I haven't seen
>>>> any other problematic methods but haven't looked for it. I just check
>>>> the one that was appearing on the profiler output.
>>>>
>>>>
>>>>
>>>>> What this function returns is a handle on an image. I fail to see the
>>>>> case
>>>>> where you'd be dereferencing this smart pointer often enough to affect
>>>>> program performance. If you are, you probably need to be using
>>>>> itk::ImageRegionIterator, or hoist calls to access methods via smart
>>>>> pointers outside of inner loops.
>>>>
>>>> I understand that, but I am not responsible for the code calling this
>>>> method. It was somewhere in the demons registration hierarchy.
>>>>
>>>> Tom
>>>>
>>>
>>
> _______________________________________________
> Insight-developers mailing list
> Insight-developers at itk.org
> http://www.itk.org/mailman/listinfo/insight-developers
>