Re: Unchecked dynamic_cast

On Saturday 07 of June 2014, Michael Stahl wrote:
> 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.

 But that's exactly the point of what Max says. The warning is not bogus. 
Using a dynamic_cast where a static_cast is known to work well is a sign of 
the un-debuggable world of defensive programming that you speak of.

-- 
 Lubos Lunak
 l.lunak@collabora.com