[Insight-developers] ImageIterators : Suggested Patch: Exception
Luis Ibanez
luis.ibanez at kitware.com
Wed May 6 07:15:55 EDT 2009
Hi Gert,
I like the option of having a Macro that
* in Debug mode will use assert
* in Release mode will throw and exception.
--------
Coming back to specific problem at hand:
A Nightly build was run with this
patch for Iterators and 26 tests failed:
http://www.cdash.org/CDash/viewTest.php?onlyfailed&buildid=327351
About 12 of them are related to Mathematical Morphology classes
in Code/Review.
Others are in unexpected places, like:
* itkImageSeriesWriterTest
* itkCommonPrintTest
It seems that this is a fundamental flaw that we want to
fix before releasing ITK 3.14 by the end of this month.
Luis
-------------------
Gert Wollny wrote:
> 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
>
>
>
More information about the Insight-developers
mailing list