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

[Williams, Norman K]

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