[prev in list] [next in list] [prev in thread] [next in thread] 

List:       koffice-devel
Subject:    Re: Review Request: Add struct KoFontFace
From:       Thorsten Zachmann <t.zachmann () zagge ! de>
Date:       2009-11-29 5:13:13
Message-ID: 200911290613.13501.t.zachmann () zagge ! de
[Download RAW message or body]

Hello Inge,

On Saturday 28 November 2009 13:57:19 Inge Wallin wrote:
> > On 2009-11-26 04:51:54, Thorsten Zachmann wrote:
> > > trunk/koffice/libs/odf/KoGenStyles.cpp, line 320
> > > <http://reviewboard.kde.org/r/2257/diff/2/?file=15087#file15087line320>
> > >
> > >     As you already pointed out this will result in a memory leek as
> > > nobody will delete the KoFontFace objects. How about storing the
> > > KoFontFace in the fontFaces map as a object instead of a pointer like
> > > QMap<QString, KoFontFace>. Not sure if you need it again to be modified
> > > which should be avoided in my opinion. If so you can go the same way as
> > > KoGenStyles::styleForModification
> 
> Actually, it only looked like a memory leak, since KoGenStyles takes over
>  ownership and will later delete it.

There is still a memory leak as the KoFontFaces are not deleted when the 
KoGenStyles is deleted.
> 
> As for the other comment, won't that lead to very inefficient code?  The
>  class is not implicitly shared and it will lead to a lot of copies when
>  you access the list.  

Not really. All the members of KoFontFace are implicitly shared so it is not a 
problem.

>  My suggestion is that we allow this patch to commit
>  now and if there are other issues they will show themselves and we can
>  change it afterwards.

I think we should keep the class KoGenStyles consistent with the other methods 
to make it easy to use.

Thorsten
_______________________________________________
koffice-devel mailing list
koffice-devel@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel
[prev in list] [next in list] [prev in thread] [next in thread]