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

[Daniel Blezek]

In this case, I'd be happy with a "ITK_LEAN_AND_MEAN" protected check and
warning of dynamic_cast and static_cast return different results.  We don't
need to go so far as to report on all the changes, just be aware that
changes to a fundamental base class are delicate. Changing this particular,
fundamental section of code had/has far reaching implications to those who
use "bugs" and "features" intentionally, or out of ignorance.

-dan


On 8/25/09 10:11 AM, "Stephen Aylward" <stephen.aylward at kitware.com> wrote:

> 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)->GetRequestedReg
>> ion());
>>     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
>> 
>> 
> 
> 

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