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

[Bradley Lowekamp]

Kent,

There is no problem with the implicit template parameters. Even VS6 supported it for non-member functions. I am pretty sure that now all the compilers we support can also do it with member functions of classes.

Brad

On Jul 24, 2012, at 1:04 PM, Williams, Norman K wrote:

> I tried this and it appears to work, but I'm a bit scared there will be a
> compiler that freaks out about only the first template parameter being
> given; CLang++ swallows it whole:
> 
> #ifndef NDEBUG
> template <typename TTarget, typename TSource>
> TTarget itkDynamicCastInDebugMode(TSource x)
> {
>  if(x == 0)
>    {
>    return 0;
>    }
>  TTarget rval = dynamic_cast<TTarget>(x);
>  if(rval == 0)
>    {
>    itkGenericExceptionMacro(<< "Failed dynamic cast to "
>                             << typeid(TTarget).name()
>                             << " object type = "
>                             << x->GetNameOfClass());
>    }
>  return rval;
> }
> #else
> template <typename TTarget, typename TSource>TTarget
> itkDynamicCastInDebugMode(TSource x)
> {
> return static_cast<TTarget>(x);
> }
> #endif // NDEBUG
> 
> I tested it by changing itkImageToImageFilter.hxx:
> 
> template< class TInputImage, class TOutputImage >
> const typename ImageToImageFilter< TInputImage, TOutputImage
>> ::InputImageType *
> ImageToImageFilter< TInputImage, TOutputImage >
> ::GetInput(void) const
> {
> 
> -  return static_cast< const TInputImage * >( this->GetPrimaryInput() );
> +  return itkDynamicCastInDebugMode< const TInputImage * >(
> this->GetPrimaryInput() );
> }
> 
> 
> 
> --
> Kent Williams norman-k-williams at uiowa.edu
> 
> 
> 
> 
> 
> 
> On 7/24/12 9:41 AM, "Bradley Lowekamp" <blowekamp at mail.nih.gov> wrote:
> 
>> 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 <http://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
>> 
>> 
>> 
>> 
>> 
>> 
>> _______________________________________________
>> 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
> 
> 
> 
> ________________________________
> 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.
> ________________________________