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

[Luis Ibanez]

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