[Insight-developers] Re: Changes to ImageSource::GraftNthOutput

[Luis Ibanez]

Hi Jim,


I'm looking at the changes on the ImageSource::GraftNthOutput() method
and found that the code was already assuming that all the outputs are
of the same type.



Please take a look at the differences between the current version and
the previous one:

http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Common/itkImageSource.txx?root=Insight&r1=1.59&r2=1.60



The previous version (1.59) was doing roughtly the following:


ImageSource<TOutputImage>
::GraftNthOutput(unsigned int idx, TOutputImage *graft)
{
  typename ImageSource<TOutputImage>::OutputImageType * output
                                           = this->GetOutput(idx);

    output->Graft( graft );
}



So:



1) The signature of GraftNthOutput() itself is already assuming
    that the argument to be grafted is of type "TOutputImage".
    Which is incorrect for filters having multiple output images
    of different types.


2) It takes the DataObject returned by GetOutput( idx ) and
    statically casted (without using static_cast<>) into a raw
    pointer to TOutputImageType. Again, assuming that the outpu
    idx, is actually of type TOutputImageType *.  This is also
    incorrect for filters with multiple output images of different
    types.


3) It passes the second argument (of type TImageOutput *)
    to the Graft() method the "output" variable.


In this context, the addition of the dynamic_cast was appropriate,
because it is actually verifying that the output is of the
correct type, instead of forcefully casting it without verification.



It seems that what should be fixed is the type itself of the
second argument of GraftNthOutput(), which should probably be
just a (DataObject *) in order to be as general as the
GetOutput() method.


The code should look like:


ImageSource<TOutputImage>
::GraftNthOutput(unsigned int idx, DataObject *graft)
{
  DataObject * output = this->GetOutput(idx);
  output->Graft( graft );
}


Which also means that the Graft() method should be declared
virtual at the level of the DataObject, and all subsequent
derived classes should reimplement it with an internal
dynamic_cast that will verify if the argument DataObject*
is actually pointing to the expected data type.


Please let me know if you find these changes to
be appropriate, and if so, I will commit then to
the trunk and to the release branch.



   Thanks


      Luis




---------------------------------
Miller, James V (Research) wrote:
> Luis, 
> 
> You made some changes to GraftNthOutput in ImageSource that include throwing exceptions and performing a dynamic_cast.
> 
> Why was the dynamic_cast change necessary?  The output being grafted does not have to be same type as output 0.  If we are worried about the types, then it really has to be the same type as the output being grafted (indicated by idx) not output 0.  May have to check the typeid of graft and the selected output.
> 
> I am not sure the dynamic_cast as written will do anything.  The return type of GetOutput() has already been static_cast to be the same output 0 anyway.
> 
> Jim Miller 
> _____________________________________
> Visualization & Computer Vision
> GE Research
> Bldg. KW, Room C223
> 1 Research Circle, Schenectady NY 12309-1027
> 
> millerjv at research.ge.com <mailto:millerjv at research.ge.com>
> (518) 387-4005, Dial Comm: 8*833-4005
> Cell: (518) 505-7065, Fax: (518) 387-6981
> 
> 
> 
>