[Insight-developers] ExceptionObject Borland Compile errors

[Luis Ibanez]

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
> 
> 
> 
> 
>