[Insight-developers] My Dashboard Compile Errors and assert()'s

Mon Aug 11 08:59:12 EDT 2008

Shoot - hit send by accident...

I'll respond more appropriately later - sorry - gotta run...

Stephen

On Mon, Aug 11, 2008 at 8:53 AM, Stephen Aylward
<Stephen.Aylward at kitware.com> wrote:
> Keep in mind that we have three types of people involved in this scenario:
>
> 1) "ITK Developers" (aka Luis wanna-be's :) ).   We have access to ITK
> code, we don't mind looking at ITK code, and we are somewhat accepting
> of its flaws.
>
> 2) "ITK Users" are people who don't read this list, who download ITK
> from the web, and who expect ITK to be perfect :)    They should never
> have to look inside ITK .txx/.cxx files...and they really should never
> have to modify ITK's .txx/.cxx files.  They want to write programs,
> using ITK, that can be given to others.
>
> 3) "Application Users" are people who use the programs written by ITK
> Users.   They want the programs to be perfect :)    But, in
> particular, they really don't want the programs to crash.
>
> The existence of ITK Users between ITK Developers and Application
> Users is exactly why we need to use exceptions instead of asserts.
>
> ITK Users need to be able to catch an exception that is informative
> and that help them use ITK code as the ITK Developers intended.
> Worse case situation, the ITK User releases an application to a
> Application User and that application user loads/generates data that
> the ITK User didn't expect. That data is then passed to a method in
> ITK that calls another method in ITK that calls another method in ITK
> that calls abort (via assert)...and the program crashes.  If I was the
> ITK User and the program crashed in a such a manner, I'd certainly
> tell the Application User that is wasn't my fault, it was ITK's fault,
> and I'd trash the good reputation of ITK.   Also, I'd be left with
> either choosing not to use ITK or having to go through the ITK code to
> find out what is the meaning behind the abort and how my code brought
> it about.    That is, the ITK User will be forced to look at ITK's
> .cxx/.txx files...yuck.
>
> Or consider the even worseter :) case, the same above situation but
> with the code compiled in Release mode....now the ITK User is really
> out of luck...the code is not going to crash or even throw an
> exception where the error would have been detected if an exception had
> been used instead of an assert...but since the assert becomes a nop in
> release mode, the program will happily continue on with the
> out-of-bound index (for example) until some memory overwrite or other
> down-the-line failure occurs.   Such delayed and secondary
> consequences can truly be a bear to resolve...particularly if you're
> an ITK User and not an ITK developer.
>
>
> On Mon, Aug 11, 2008 at 8:50 AM, Gaëtan Lehmann
> <gaetan.lehmann at jouy.inra.fr> wrote:
>>
>> Le 11 août 08 à 13:41, Stephen Aylward a écrit :
>>
>>> Dang - I was hoping to not have to go into more detail...
>>>
>>>> When an algorithm is dependant of the content of
>>>> the manipulated data, it is very difficult to test all the cases. Keeping
>>>> the asserts in the code can highlight a problem with new data, which
>>>> would
>>>> have be very difficult to found without them.
>>>
>>> This is exactly when we should use an exception instead of an abort.
>>>
>>
>> No. The bug in SignedMaurerDistanceMapImageFilter I gave in example was in a
>> loop. Adding an exception there would have important performance impacts.
>>
>>>
>>>> Because it can be quite time consuming (when used in a loop, or to
>>>> validate
>>>> a tree property for exampe) and it exits quite badly (but that's useful
>>>> for
>>>> debugging)
>>>
>>> Regarding speed...In most ITK applications, I seriously doubt a check
>>> for an exception is going to have a notable impact on performance
>>> relative to other computation costs in ITK.   In the rare, inner-loop
>>> cases, if you feel the exception test is going to be too
>>> expensive...leave it out and then document in the .h that the program
>>> doesn't check but expects (for example) the input to be in a certain
>>> range.   This is what we've done in the past in ITK.   We shouldn't
>>> change our policy now.   A abort is not a solution.
>>
>> Of course it can be a speed problem. The tree property example given above
>> is a good example: checking that the values associated to all the nodes of a
>> tree made of thousands of nodes have a given property can be as long as
>> computing those values.
>> That's something we won't check in user application because that check takes
>> time, and because the code must produce a valid tree.
>>
>> However, if the user application crash or give unexpected results, the
>> developer of that application would be pleased to have some starting point
>> to debug, only by turning debug mode on. If its because he didn't respect a
>> documented precondition, he can identify a bug on his side. Otherwise, he
>> can make a bug report, and claim that ITK is not perfect to his client - in
>> that case, it would be true.
>>
>> And the user will never see the abort, because he only use the program built
>> in Release mode.
>>
>>>
>>>> Exception would also be a way to do the same job, but it requires to
>>>> carefully catch exception in ITK code, to not mask the exception sent by
>>>> the
>>>> assert-like method - is it really useful?
>>>
>>> Exception are more informative than asserts.
>>
>>
>> I'm not fully sure, but I think that an exception catched by some code and
>> resent after having performed some needed operation would lead to the lost
>> of the call stack in the debugger. Only the call stack from the second throw
>> would be visible.
>> Can someone confirm that?
>>
>> I agree that some check should be implemented with exceptions rather than
>> asserts, but I don't think we should try to remove all the assert() from
>> ITK. They are both useful and safe when used as they should.
>>
>> BTW, maybe we should have a macro to replace the asserts, when possible, and
>> give an explicit message in a concise way - something like:
>>
>> #define itkAssertMacro(cond, x) \
>>  { \
>>  if( !(cond) ) \
>>    { \
>>    ::itk::OStringStream message; \
>>    message << "itk::ERROR: " << this->GetNameOfClass() \
>>            << "(" << this << "): " x; \
>>    ::itk::ExceptionObject e_(__FILE__, __LINE__,
>> message.str().c_str(),ITK_LOCATION); \
>>    throw e_; /* Explicit naming to work around Intel compiler bug.  */ \
>>    } \
>>  }
>>
>>
>> It would give something like that in the code:
>>
>>  itkAssertMacro( ptr != NULL, << "Input image can't be null." );
>>  itkAssertMacro( v > 1, << "Distance must be greater than 1." );
>>  itkAssertMacro( label != m_BackgroundValue, << "Foreground can't have the
>> same label than background: " << label );
>>
>> Regards,
>>
>> Gaëtan
>>
>>
>>
>> --
>> 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
>>
>>
>
>
>
> --
> Stephen R. Aylward, Ph.D.
> Chief Medical Scientist
> Kitware, Inc. - North Carolina Office
> http://www.kitware.com
> (518) 371-3971 x300
>

-- 
Stephen R. Aylward, Ph.D.
Chief Medical Scientist
Kitware, Inc. - North Carolina Office
http://www.kitware.com
(518) 371-3971 x300