Re: [lmi] Safe and consistent dereferencing and casting [Was: Code review: product editor]

[Greg Chicares]

On 2007-04-12 22:43Z, Vadim Zeitlin wrote:
> On Fri, 23 Mar 2007 22:59:33 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> [...] Here's a different idea:
> GC> 
> GC> -    return dynamic_cast<wxFrame&>(*GetFrame());
> GC> +    return safely_dereference_as<wxFrame>(GetFrame());

I had independently written my own implementation. I needed it to
fix some potential undefined behavior in the production system,
and it looks much like yours anyway, so I've committed it to HEAD
for testing--which is not to say it's immutable or can't be
discussed further.

>  First I'd like to note that by using a function here you won't be able to
> get the file name and line number of the place where the cast failed. Of
> course, it's not a big problem as there is a standard solution: just define
> a macro which would call this safely_dereference_as function with __FILE__
> and __LINE__ parameters which it would then use to construct the error
> message if the cast fails.

Let me restore some context from my original email:

| I look in HEAD for places where I've already used or suggested a
| cast to reference:
[...exhaustive list...]
| Please tell me your opinion, because I'm thinking of implementing
| that soon. It seems clear to read. The function template can deduce
| its argument type. Using RTTI for the two types in the error message
| would make it easy to find the offending code: the only case that
| RTTI wouldn't suffice to disambiguate is
[...one single case...]
| and there I was wondering whether static type checking would be
| possible

So I don't believe we need a macro, but I've documented the idea
inline:

/// Alternatives considered: A macro might have been used to report
/// __LINE__ and __FILE__. We prefer to avoid macros in general. This
/// function template is intended to guard against logic errors, which
/// should be rare. It is easy enough to add a breakpoint, e.g.
///   asm("int $3"); // x86 specific.
/// if the cause of one of those rare errors is not immediately clear.

>  Anyhow, in spite of this small problem I still think that implementing
> this function would be a good idea we've just had quite a discussion with
> Evgeniy about how exactly should it be implemented. This matters because
> there are other similar wrapper functions which we need (notably the
> function for safely dereferencing a boost::any object)

That's somewhat different from downcasting a wx base-class
pointer in order to call a derived-class function, so let's
discuss it separately. I think the technique we're discussing
here would work in that case without much modification; but
I have another idea I want to explore for the downcasting case.

> and it makes sense
> that they should be implemented in a similar way so it would be great if we
> could agree on The One True way.
> 
>  Basically there are 2 options for implementing this function:
> 
>       template <typename T, typename U>
>       T& safely_dereference_as(U *ptr)
>       {
>               try
>               {
>                       if ( ptr )
>                               return dynamic_cast<T>(*ptr);
>               }
>               catch ( std::bad_cast& e )
>               {
>                       // nothing to do here
>               }
> 
>               fatal_error() << ... ;
>       }
> 
> or
> 
>       template <typename T, typename U>
>       T& safely_dereference_as(U *ptr)
>       {
>               T *p = dynamic_cast<T *>(ptr);
>               if ( !p )
>                       fatal_error() << ... ;
> 
>               return *p;
>       }
> 
> I strongly prefer the second form as I don't like provoking an exception
> which we then immediately proceed to catch and ignore but Evgeniy thinks
> that the first version is more clear (while for me it's exactly the
> contrary). What do you think about this?

I favor the second. I'm not sure I understand the motivation for
the first (is this java's 'try...catch...finally'? or a DBC idea
from some other language?), but I see some challenges that it
would need to meet.

Issues with the particular implementation above:

 - This construct seems suspicious and brittle:
     try{something();}
     catch(one_particular_exception_type&) {
   I'm not satisfied that std::bad_cast is the only exception
   that could possibly be thrown in the try-block; and if I'm
   wrong, then why wouldn't 'catch(...)' be at least as good?
   This feels kind of like a 'switch' with no 'default:'.

 - Swallowing an exception with a do-nothing catch-clause is
   an idiomatic way to make sure no exception can escape from
   a dtor. Otherwise, I think it's generally considered to
   require a specific rationale, which I don't yet see here.

Issues with the general idea:

 - 'try...catch' might be slow: probably on msw today due to
   stabs, and maybe on some other platform that could become
   important to us later. (I didn't measure it, but I committed
   a unit test to which timings can be added.)

 - Common practice is to use exceptions for exceptional cases.
   AIUI, the consensus among experts is to write 'try...catch'
   less frequently in general anyway--for example:
     http://www.gotw.ca/gotw/065.htm
     | exception safety is rarely about writing 'try' and
     | 'catch' -- and the more rarely the better.
     http://www.ddj.com/dept/cpp/184403815
     | no explicit try catch [...] briefer and clearer than
     | exception-aware code
   We can choose to do otherwise if we have a good rationale;
   I don't yet see one here.

I'm saying not that it's impossible to answer those objections,
but only that they would need to be answered. There may be a
valuable idea here that I just don't understand well enough.