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.lu...@collabora.com
_______________________________________________
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
