[Insight-developers] GetInverse is unusable -- patch attached

[Tom Vercauteren]

To add some information about my reason A to do the step 3 below. I
believe it is related to the so-called hiding rule:
http://www.parashift.com/c++-faq-lite/strange-inheritance.html#faq-23.9

Another option besides implementing "GetInverse(Self*)" in the derived
class would have been to add
  using Superclass::GetInverse;
in the header of the derived classes.

Since I already gave Luis a hard time with my previous commits that
broke either MSVC or Sun CC builds. I am not sure I am ready to commit
"advanced" c++ code such as this "using Superclass::GetInverse" :)

Tom



On Wed, Apr 8, 2009 at 19:22, Tom Vercauteren <tom.vercauteren at m4x.org> wrote:
> Luis and Paul,
>
> Sorry if I wasn't clear there.
>
> What I did was:
>
> 1) Take Pavel's patch for ITK 3.12 (i.e. implementation of "virtual
> Base::Pointer GetInverse(void)", were I only put "void" to explicitly
> say it does take an argument).
>
> 2) Extend it so that all transforms that already had either a
> "GetInverse(Self*)" or a "CloneInverseTo(Pointer &)" method now also
> have a "GetInverse(void)" method
>
> 3) Added a "GetInverse(Self*)" method for classes that did not have it
> but whose parent had one.
>
> 4) Extended all unit tests that were checking or simply calling either
> "GetInverse(Self*)" or "CloneInverseTo(Pointer &)" to also check (or
> call respectively) "GetInverse(void)"
>
>
> What might seem confusing is step 3. I did it for three reasons:
>
> A) My compiler refuses to compile "d->GetInverse(invd)" if
>       - "d" and "invd" are of type D
>       - D inherits from B
>       - D does not redefine the non-virtual method "GetInverse(Self*)"
>       - "GetInverse(Self*)" is of course defined in the base class B
>       - The virtual function "GetInverse(void)" is of course defined
> in the base class B and possibly in D
>
> B) Defining "GetInverse(Self*)" in thies derived class D allowed me to
> avoid code duplication when implementing a "GetInverse(void)" the
> returns a B::Pointer that actually points to a concrete object of the
> derived type D
>
> C) Having "GetInverse(Self*)" in more classes makes ITK more coherent
> and adresses bug http://www.itk.org/Bug/view.php?id=3359
>
>
> Hope this helps,
> Tom
>
> On Wed, Apr 8, 2009 at 18:53, Luis Ibanez <luis.ibanez at kitware.com> wrote:
>>
>> Paul,
>>
>> Thanks for the clarification...
>>
>> It seems that were are mixing patches here.
>>
>> My comments relate to the file
>>
>> file icon itk-getinverse.patch
>>
>> that Tom attached to bug:
>> http://www.kwwidgets.org/Bug/bug_view_advanced_page.php?bug_id=7876
>>
>>
>> Tom:
>>
>>  Where did you take this patch file from ?
>>
>>  Is it supposed to be equivalent to the one that Paul just posted ?
>>
>>
>>
>> Paul:
>>
>>  Regarding the objection. I failed to indicate that the objection
>>  is not about the Self type but about the use of a raw pointer.
>>  I'm suggesting to use a SmartPointer as argument so that we can
>>  allocate (construct) the transform if necessary.
>>
>>  I agree with you that the "Self" type should be replaced with a
>>  TransformBase type.
>>
>>
>>
>> ---
>>
>>
>>
>> Tom,
>>
>> let's make sure that we take the proper version of the patch...
>>
>>
>>
>>   Luis
>>
>>
>>
>>
>> --------------------
>> Paul Koshevoy wrote:
>>>
>>> Luis Ibanez wrote:
>>>
>>>> Hi Tom,
>>>>
>>>> Here are some observations about the patch: itk-getinverse.patch
>>>>
>>>> ----
>>>>
>>>> In line 184:
>>>>
>>>> CenteredRigid2DTransform<TScalarType>::
>>>> +GetInverse( Self* inverse) const
>>>> +{
>>>> +  if(!inverse)
>>>> +    {
>>>> +    return false;
>>>> +    }
>>>>
>>>>
>>>> It is tempting to use a SmartPointer as argument
>>>> so that we can do:
>>>>
>>>> +GetInverse( Self::Pointer inverse) const
>>>> +{
>>>> +  if( inverse.IsNull() )
>>>> +    {
>>>> +    inverse = Self::New();
>>>> +    }
>>>>
>>>>
>>>
>>>
>>>
>>> Hi Luis,
>>>
>>> It seems the code you are objecting to is the existing ITK code.  Passing
>>> a Self pointer (smart or not) restricts the inverse transform type to be the
>>> same type as the forward transform.  That is broken, because only a few
>>> transforms have an analytic inverse of the same type.  This is not what my
>>> patch does, although I have followed the style and implemented GetInverse()
>>> inline in the .h file.
>>>
>>> My current patch doesn't have the changes you are referring to:
>>>
>>> https://code.sci.utah.edu/svn/ImageReconstruction/trunk/code/itk-patch/InsightToolkit-3.12.0-GetInverse.patch
>>>
>>>   Pavel.
>>>
>>>
>>
>