[Insight-developers] ImageIterators : Suggested Patch: Exception

Wed May 6 06:34:41 EDT 2009

On Tue, 2009-05-05 at 19:53 +0200, Gaëtan Lehmann wrote:
> Hi,
> 
> It would be nice to have that check in the code.
> I think it's better to not use assert() here because it can let the  
> user to catch problematic mistakes in it's code and the check is not  
> so computing intensive, so I doubt we can see any difference.
Well, since the error is a programing error, the reason to use assert
over throwing an exceptions is that with an assertion, you can open a
debugger and end up exactly at the offending code line [1] - either by
starting the debugger directly, like Visual Studio does, or by examining
the core dump on a Unix-like system, and you can see directly, where the
bad input came from. 

When an exception is thrown, the stack is unwind, and you have a hard
time to find the source of the offending data. The only approach here is
to explicitly open a debugging session, and then configure the debugger
to catch exceptions when they are thrown.

One idea to check also in the release builds would be that in debug
builds the code uses assert, and in release builds it throws an
exception, so that a developer can get easily to the offending
code/data, and a user sees a reasonable error message without a core
dump or general protection fault. The other option would be to define an
assert that is also compiled in when NDEBUG is defined.

[1] H. Sutter & A. Alexandrescu, "C++ Coding Standards"

Regards, 

Gert 

> Regards,
> 
> Gaëtan
> 
> 
> 
> Le 5 mai 09 à 18:14, Luis Ibanez a écrit :
> 
> >
> > 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 
> >> >
> > _______________________________________________
> > 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