Unchecked dynamic_cast

[Michael Stahl]

On 06/06/14 13:41, Max Kellermann wrote:
> Hi Caolan,
> 
> I just happened to read your Libreoffice commits while browsing the
> git repository.  For example:
> 
>              if ( pFmt->Which() == RES_FLYFRMFMT )
>              {
> -                GetDoc()->SetFlyFrmTitle( *(dynamic_cast<SwFlyFrmFmt*>(pFmt)),
> +                GetDoc()->SetFlyFrmTitle( dynamic_cast<SwFlyFrmFmt&>(*pFmt),
>                                            rTitle );
> 
> This, however, is still wrong.  It just hides the warning.
> 
> We know already at this point that pFmt is not NULL.  But what we
> don't know is whether pFmt is a SwFlyFrmFmt instance.  dynamic_cast
> can return NULL even if its parameter is not NULL.
> 
> So, you have checked that its type is RES_FLYFRMFMT, and thus it must
> be a SwFlyFrmFmt instance.  Ok, but then you don't need the overhead
> of dynamic_cast.  A static_cast will do the same, with less runtime
> overhead.
> 
> In any case, your changes do not actually address the Coverity
> warnings; they merely hide them.
> 

the warning above is bogus - the check on Which() is effectively a type
check, so any change that shuts up the warning is good enough; and i bet
the "runtime overhead" of this line would never show up in a profile anyway.

actually i'd be annoyed if this were changed to check if the
dynamic_cast was successful - that has the potential to take us further
into the un-debuggable world of defensive programming, where nobody
knows what the invariants of some class or function actually are.