[Insight-developers] STYLE: Un-necessary use of static_cast -- should we care?

[Bradley Lowekamp]

Kent,

I think I am leaning towards recommending that dynamic_cast be use. 

1) Compared to the other overhead in the GetInput methods will be a very small fraction.

2)  The ImageSource::GetOutput and the ImageToImage::GetInput, already have a dynamic_cast in it.

3) I have encountered at least 5 filters which were using incorrect types with the GetInput/Output methods... They work until the point something changed in ITK and they stopped.

As I am not a fan of macro, and try to use the so much more easy to read an understand template functions :) Please consider using a template function as opposed to a macro.

Please also see the recently updated GetOutput method, which includes a couple cases recommended by Jim.

template< class TOutputImage >
typename ImageSource< TOutputImage >::OutputImageType *
ImageSource< TOutputImage >
::GetOutput(unsigned int idx)
{
  TOutputImage *out = dynamic_cast< TOutputImage * >
                      ( this->ProcessObject::GetOutput(idx) );

  if ( out == NULL && this->ProcessObject::GetOutput(idx) != NULL )
    {
    itkWarningMacro (<< "Unable to convert output number " << idx << " to type " <<  typeid( OutputImageType ).name () );
    }
  return out;
}



On Jul 24, 2012, at 10:23 AM, Williams, Norman K wrote:

> I'm not %100 sold on going to dynamic cast for GetInput, myself.  I will
> say that if we key off the build type, the performance hit would be
> limited to Debug builds.
> 
> But a real world scenario that this would help with: I recently did some
> work on a filter that was written with ITK3 that didn't work on ITK4.  The
> problem was that whoever wrote the filter assigned an optional input to
> input 0 of the filter. ITK4 checks for a missing input 0, meaning the
> filter threw an exception before actually doing any work.
> 
> So I went through the filter and re-assigned the inputs such that the
> required input was input 0, and the other inputs were re-numbered.  In the
> course of that, I missed one place where I should have changed indices,
> and Hans ended up having to spend time debugging the problem.  As it
> happened, inputs 1 and 2 have different image types, but since static_cast
> was used to return them, it was silently using swapped inputs and
> producing nonsense.
> 
> That's the case that dynamic_cast would address -- fumblefingers
> programmers (e.g. me) putting the wrong objects in the wrong input slots.
> 
> On the other hand, the replacement for static_cast would require
> considerably more work, for example:
> 
> template< class TInputSpatialObject, class TOutputImage >
> const typename SpatialObjectToImageFilter< TInputSpatialObject,
> TOutputImage >::InputSpatialObjectType *
> SpatialObjectToImageFilter< TInputSpatialObject, TOutputImage >
> ::GetInput(unsigned int idx)
> {
> #if BUILD_TYPE_RELEASE // actually all this would go in a macro
>  return static_cast< const TInputSpatialObject * >
>    ( this->ProcessObject::GetInput(idx) );
> #else
>  DataObject *rval = this->ProcessObject::GetInput(idx);
>  if(rval == 0) // unassigned
>    {
>    return 0;
>    }
>  const TInputSpatialObject *rval2 =
>    dynamic_cast<const TInputSpatialObject *>(rval);
>  if(rval2 == 0)
>   {
>   itkExceptionMacro(<< "Cast failed, wrong object type "
>                     << rval->GetNameOfClass());
>   }
>  return rval;
> #endif
> }
> 
> Another point in favor of using dynamic_cast: the performance penalty of
> using it only matters if you call it a lot.  I just spent a week getting
> rid of code that did this.
> 
> 
> 
> ________________________________
> Notice: This UI Health Care e-mail (including attachments) is covered by the Electronic Communications Privacy Act, 18 U.S.C. 2510-2521, is confidential and may be legally privileged.  If you are not the intended recipient, you are hereby notified that any retention, dissemination, distribution, or copying of this communication is strictly prohibited.  Please reply to the sender that you have received the message in error, then delete it.  Thank you.
> ________________________________
> _______________________________________________
> Powered by www.kitware.com
> 
> Visit other Kitware open-source projects at
> http://www.kitware.com/opensource/opensource.html
> 
> Kitware offers ITK Training Courses, for more information visit:
> http://kitware.com/products/protraining.php
> 
> Please keep messages on-topic and check the ITK FAQ at:
> http://www.itk.org/Wiki/ITK_FAQ
> 
> Follow this link to subscribe/unsubscribe:
> http://www.itk.org/mailman/listinfo/insight-developers

========================================================
Bradley Lowekamp  
Medical Science and Computing for
Office of High Performance Computing and Communications
National Library of Medicine 
blowekamp at mail.nih.gov



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.itk.org/pipermail/insight-developers/attachments/20120724/5e23f478/attachment.htm>