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

[Stephen Aylward]

Yes - that is breaking backward compatibility....BUT....

It is a great example of how one persons "bug" is another persons
"feature."  However, this bug was causing ITK to crash for some
folks...so...who should "win."

Any ideas on how to address this?

I think it is a really important problem.

Perhaps the real issue is notification?   Wouldn't it be great if when
we re-compile using a new version of ITK, it sends us a warning if any
of the methods we called contained code that had changed, and lets us
know how the code was changed...Could possibly be done via a macro
inserted where ever code is changed (macro would take the revision
number and a message as args).  If you run the code with a "show me
what has changed since revisions X" compile-time option, then when
changed code is encountered, it posts the message.

The issue is that our code could get ugly very fast.

Any other ideas?

s




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



-- 
Stephen R. Aylward, Ph.D.
Director of Medical Imaging
Kitware, Inc. - North Carolina Office
http://www.kitware.com
stephen.aylward (Skype)
(919) 969-6990 x300