[Insight-developers] Broken backwards compatibility -- results of 2 days debugging

[Bill Lorensen]

At a minimum that fix should have included a warning if the dynamic
cast failed. Also, Lucas' code should be fixed to check for the NULL.


Bill

On Tue, Aug 25, 2009 at 10:42 AM, Daniel Blezek<Blezek.Daniel at mayo.edu> wrote:
> Hi  all,
>
>   I had integrated Luca Antiga’s vesselness code into our application a
> while ago.  Fairly recently, it has been breaking on one of our platforms,
> the only one building with the CVS head version of Insight.  I finally
> tracked the crash down to this sequence.
>
> Luca’s code creates output images of different types and caches them in the
> outputs:
>   typename HessianImageType::Pointer hessianImage = HessianImageType::New();
>   this->ProcessObject::SetNthOutput(2,hessianImage.GetPointer());
>
> Later, he grabs the output and allocates the buffers:
>     this->GetOutput(2)->SetBufferedRegion(this->GetOutput(2)->GetRequestedRegion());
>     this->GetOutput(2)->Allocate();
>
> This code crashes due to this change:
> revision 1.64
> date: 2009-03-10 09:38:35 -0500;  author: blowekamp;  state: Exp;  lines:
> +24 -10;  commitid: 0s3FC2A2iqP1NuFt;
> BUG: 8681 ImageSource lacks type safety for multiple outputs of different
> types. Changed static_cast to dynamic_cast in GetOutput(unsigned int), so
> that we don't return an invalid pointer. GraftNthOutput now uses type safe
> ProcessObject::GetOutput. AllocateOutputs previously would segfault under
> these conditions. Now it safely casts to ImageBase and calls Allocate
> methods.
>
> which made this code use dynamic_cast, rather than static_cast:
>
>   return dynamic_cast<TOutputImage*>
>     (this->ProcessObject::GetOutput(idx));
>
>
> So now, my GetOutput(2) returns null (cause it’s not a TOutputImage) and
> everything crashes.
>
> This was highly frustrating to me.  Yes I know this is a bug, and I know it
> should be fixed, but code which depended on this behavior (Luca’s first
> implementation) is now broken.  Does this constitute a break of backwards
> compatibility?
>
> I propose at least a warning if the static_cast and dynamic_cast return
> different pointers.  This would have saved me two days of deep debugging and
> wondering why the same code ran fine on an older release version of ITK.
>
> Best regards,
> -dan
>
>
>
> --
> Daniel Blezek, PhD
> Medical Imaging Informatics Innovation Center
>
> P 127 or (77) 8 8886
> T 507 538 8886
> E blezek.daniel at mayo.edu
>
> Mayo Clinic
> 200 First St. S.W.
> Harwick SL-44
> Rochester, MN 55905
> mayoclinic.org
>
>
> _______________________________________________
> Powered by www.kitware.com
>
> Visit other Kitware open-source projects at
> http://www.kitware.com/opensource/opensource.html
>
> 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
>
>