[Insight-developers] ImageIterators : Suggested Patch: Exception
Gaëtan Lehmann
gaetan.lehmann at jouy.inra.fr
Fri May 8 03:46:33 EDT 2009
Hi Luis,
I'll try to fix that this weekend, and to fix the various problems
found in those classes as well.
Regards,
Gaëtan
Le 6 mai 09 à 13:15, Luis Ibanez a écrit :
>
> 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
--
Gaëtan Lehmann
Biologie du Développement et de la Reproduction
INRA de Jouy-en-Josas (France)
tel: +33 1 34 65 29 66 fax: 01 34 65 29 09
http://voxel.jouy.inra.fr http://www.mandriva.org
http://www.itk.org http://www.clavier-dvorak.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PGP.sig
Type: application/pgp-signature
Size: 186 bytes
Desc: Ceci est une signature ?lectronique PGP
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20090508/8044dc4e/attachment.pgp>
More information about the Insight-developers
mailing list