[Insight-developers] ExceptionObject Borland Compile errors

[Bill Lorensen]

Guys,

itkExceptionObjectTest is now failing on some platforms:

http://www.cdash.org/CDash/testSummary.php?project=2&name=itkExceptionObjectTest&date=20080530

Bill

On Thu, May 29, 2008 at 5:45 PM, Luis Ibanez <luis.ibanez at kitware.com> wrote:
>
> Hi Niels,
>
> Thanks for committing the fix.
>
> The issue with c_str() is that Borland requires the two
> arguments of the ternary operator to be of the same type.
>
> So Borland will complain for expressions such as:
>
>      () ?  std::string : char *
> and   () ?  char *      : std::string
>
>
> The original code was:
>
>       () ?  " "  : m_File
>
> where m_File is an std::string, so it was of type
>
>       () ?  char *      : std::string
>
>
> I changed it to
>
>       () ?  " "  : m_File.c_str()
>
> to make it
>
>      () ?  char * : char *
>
> ---
>
> Your suggestion of changing it to
>
>       () ?  std::string()  : m_File
>
> is fine, since it will match
>
>      () ?  std::string : std::string
>
>
> However,
>
> Today is a bad time for committing further
> changes, specially these ones that are
> rather cosmetic.
>
> Would you mind committing them after
> 9:00pm New York time ?  That the time
> at which the Nightly build closes, and
> we open tomorrow's continous Dashboard.
>
> In this way, you will see the effect of
> your commits during the continuous builds,
> as opposed as the entire set of Nightlies.
>
> --
>
> A perfomance penalty in the dynamic_cast
> shouldn't be a great concern here given
> the we probably will not be throwing
> exceptions at a continous and/or high rate.
>
> Adding a comment on that line, describing
> the trade-offs between the two castings
> will be a good idea.
>
>
>
>   Thanks a lot for helping
>   to improve the toolkit !
>
>
>
>      Luis
>
>
>
> ---------------------
> Niels Dekker wrote:
>>
>> Luis Ibanez wrote:
>>
>>> Please feel free to fix the typo:
>>>   ExceptionObject::SetLocation( description )
>>
>>
>> Done:
>>
>> www.itk.org/cgi-bin/viewcvs.cgi/Code/Common/itkExceptionObject.cxx?root=Insight&r1=1.13&r2=1.14
>>
>> I only did some simple local tests; I think that should be sufficient,
>> because the fix is so straight-forward.  But I won't make any other changes
>> before having submitted another Experimental build.  Which takes me quite a
>> few hours, unfortunately...  (So it won't be today anymore.)
>>
>>> and to make the improvement of adding a local
>>> variable to avoid calling GetExceptionData()
>>> multiple times.
>>
>>
>> Thanks  :-)  As I said, it will take some more time, but it isn't as
>> urgent as the typo found by Karthik.
>>
>>> BTW: please note that the c_str() calls *are* needed
>>> by Borland, so please keep those ones in place.
>>
>>
>> I do understand that you'd rather not want any more Borland errors for a
>> while. But I still wonder why Borland would need the c_str() calls.  I
>> thought it was because the operands at the right of the
>> question-mark-operator had a different type, in my original version:
>>
>>  IsNull ? "" : m_ExceptionData->m_File
>>
>> In such cases, my copy of Borland 5.5.1 appears to say "Error E2354: Two
>> operands must evaluate to the same type".  Apparently because "" is a
>> char-pointer, while m_File is an std::string.  This issue could be fixed,
>> simply by having an std::string for both operands, e.g.
>>
>>  IsNull ? std::string() : m_ExceptionData->m_File
>>
>> In the new version, it would look more like this:
>>
>>  thisData ? thisData->m_File : std::string()
>>
>> Anyway, if you want the c_str() in there, no problem, but I don't think
>> it's really necessary...
>>
>>> I would rather keep the dynamic_cast<> because
>>> when the pointer is not of the appropriate type,
>>> at least a NULL pointer will be produced, while
>>> the static_cast will convert the pointer even
>>> if no conversion is valid.
>>
>>
>> Okay, in that case I think it's just a matter of taste. I would do
>> static_cast, to state explicitly that the pointer is in fact an
>> ExceptionData pointer. Because it's hard-coded that way. While you'd rather
>> double-check, just to be sure.  Which definitely makes sense as well, so
>> I'll leave the dynamic_cast in there.
>>
>>> Do you see any drawback in the use of dynamic_cast ?
>>
>>
>> dynamic_cast is known to have some performance penalty.  But I don't
>> expect it to be very large, in this particular case. Anyway, there's more
>> about the performance of dynamic_cast in the Technical Report on C++
>> Performance: www.research.att.com/~bs/performanceTR.pdf
>>
>>> Changing the return type of GetExceptionData()
>>> sounds fine.
>>
>>
>> Thanks  :-)
>>
>>> Also, If you have a chance to verify that this is
>>> still solving the original issue related to crashes
>>> at the moment of throwing exceptions, that will be
>>> great.
>>
>>
>> Good point. I should think about how to unit-test it!
>>
>> Good night (good nightly build!),
>>
>>  Niels
>>
>>
>>
>>
>>
> _______________________________________________
> Insight-developers mailing list
> Insight-developers at itk.org
> http://www.itk.org/mailman/listinfo/insight-developers
>