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

[Luca Antiga]

Dan, Steve, Bill,
  indeed this was a bug with my code, surely ascribable to ignorance  
in this case. Should have checked twice and not just be happy with the  
fact that the code worked.

Indeed Bradley had fixed the incriminated  
MultiScaleHessianBasedMeasureImageFilter code in Review on Feb 25 and  
Mar 10 (and the ImageSource fix is Mar 10), so the Review version  
works. Only the way the class is templated has changed from the IJ  
submission to Review, so Dan you'll have to adapt your code if you go  
with the Review version.

This is for this particular case. Of course, the point you just raised  
has a broader scope.

I agree that a warning in ImageSource is in order.

Luca


On Aug 25, 2009, at 6:04 PM, Bill Lorensen wrote:

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

--
Luca Antiga, PhD
Head, Medical Imaging Unit,
Biomedical Engineering Department,
Mario Negri Institute.
mail: Villa Camozzi, 24020, Ranica (BG), Italy
phone: +39 035 4535-381
email: antiga at marionegri.it
web: http://villacamozzi.marionegri.it/~luca

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20090825/7ef18238/attachment.htm>