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

Mon Aug 11 08:53:43 EDT 2008

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