[Insight-developers] ImageIterators : Suggested Patch: Exception

[Luis Ibanez]

Hi Brad,


Thanks for the feedback.


We don't have guidelines for asserts nor preconditions in ITK.


Although,
unless I'm missing something,
that will simply mean that instead of writing:


    if( ! bufferedRegion.IsInside( m_Region ) )
       {
       itkGenericExceptionMacro("..");
       }


we will write


    assert( bufferedRegion.IsInside( m_Region ) )


and the implementation of assert witll throw the exception.

Which is admitedly a more elegant way of writing the code.


We could achieve a similar goal with itkTestingMacros such
as the ones we have been using in

http://svn.na-mic.org/NAMICSandBox/trunk/QuadEdgeMeshRigidRegistration/Testing/itkTestingMacros.h


Would you recommend a better approach ?


    Thanks


       Luis



------------------------
Bradley Lowekamp wrote:
> I ran into this issue with trying to improve the ShrinkImageFilter. I 
> think this is a good check to make. In my case we only had test failing 
> on one or two systems, even through the buffer was being accessed 
> outside the buffered region.
> 
> However, this seems like an assertion of preconditions. In this case the 
> precondition is that the region parameter must be with in the buffered 
> region. There are a variety of ways to verify preconditions. One way is 
> to use an assert type macro that is only implemented in Debug versions. 
> Do we have any guidelines for asserts or verification of preconditions 
> in ITK?
> 
> Brad
> 
> 
> On May 5, 2009, at 11:07 AM, Luis Ibanez wrote:
> 
>> With Michel Audette we have been tracking the issues related to
>> aligning physical point coordinates with index coordinates.
>>
>> In the process we noticed that the ITK image iterators do not
>> verify at construction time whether the region given as argument
>> to the iterator constructor,  is inside of the image or not.
>>
>> The consequence is that a user can provide a region outside
>> of the image, we compute the offset (that will turn out to be
>> outside of the image buffer) and then we (incorrectly) provide
>> access to the pixel location.
>>
>> This has now been reported as Bug #8960:
>>
>>     http://public.kitware.com/Bug/view.php?id=8960
>>
>>
>> We are suggesting the following fix to this problem:
>>
>> Namely, to check the region argument of the iterator constructor
>> against the BufferedRegion of the image, and throw an exception
>> if the region is not fully inside of the image buffered region.
>>
>>
>> Index: itkImageConstIterator.h
>> ===================================================================
>> RCS file: /cvsroot/Insight/Insight/Code/Common/itkImageConstIterator.h,v
>> retrieving revision 1.23
>> diff -r1.23 itkImageConstIterator.h
>> 175a176,183
>>
>>>    const RegionType & bufferedRegion = m_Image->GetBufferedRegion();
>>
>>>
>>>    if( ! bufferedRegion.IsInside( m_Region ) )
>>
>>>      {
>>
>>>      itkGenericExceptionMacro("Region " << m_Region
>>
>>>        << " is outside of buffered region " << bufferedRegion );
>>
>>>      }
>>
>>>
>> Index: itkImageConstIteratorWithIndex.txx
>> ===================================================================
>> RCS file: 
>> /cvsroot/Insight/Insight/Code/Common/itkImageConstIteratorWithIndex.txx,v
>> retrieving revision 1.27
>> diff -r1.27 itkImageConstIteratorWithIndex.txx
>> 84a85,92
>>
>>>  const RegionType & bufferedRegion = m_Image->GetBufferedRegion();
>>
>>>
>>>  if( ! bufferedRegion.IsInside( m_Region ) )
>>
>>>    {
>>
>>>    itkGenericExceptionMacro("Region " << m_Region
>>
>>>      << " is outside of buffered region " << bufferedRegion );
>>
>>>    }
>>
>>>
>>
>>
>> Does anyone see a problem with this patch ?
>>
>> Any objections ?
>>
>>
>>   Thanks
>>
>>
>>        Luis
>> _______________________________________________
>> Powered by www.kitware.com <http://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
> 
> 
> 
> 
> ========================================================
> 
> Bradley Lowekamp  
> 
> Lockheed Martin Contractor for
> 
> Office of High Performance Computing and Communications
> 
> National Library of Medicine 
> 
> blowekamp at mail.nih.gov <mailto:blowekamp at mail.nih.gov>
> 
>