Re: [Libreoffice] [PATCH] Replace List with std::vector< ImplBmpObj* >

On 19 July 2011 14:22, Joseph Powers <JPOWERS27@cox.net> wrote:
> A List<> would  be better; however, it's a list of pointers so the size isn't that big.
So why not use it ?
I did not mean the actual size in memory but the number of elements.
I've seen there around 150 elements when I tried to print the size.
That's not really much but I think when we can use something better we should.
I don't really know how many elements there can be and how often we
are removing not from the end and what's the real difference in
effectiveness between list and vector but..

May be someone has opinion about this?

> > void ImplSalBitmapCache::ImplRemove( X11SalBitmap* pBmp )
> > {
> > -    for( ImplBmpObj* pObj = (ImplBmpObj*) maBmpList.Last(); pObj;
> > pObj = (ImplBmpObj*) maBmpList.Prev() )
> > +    for( size_t i = maBmpList.size(); i; )
> >     {
> > +        ImplBmpObj* pObj = maBmpList[ --i ];
> >         if( pObj->mpBmp == pBmp )
> >         {
> > -            maBmpList.Remove( pObj );
> > +            maBmpList.erase( maBmpList.begin() + i );
> >             pObj->mpBmp->ImplRemovedFromCache();
> >             mnTotalSize -= pObj->mnMemSize;
> >             delete pObj;
> >
> > But then I realized you are decreasing i in ImplBmpObj* pObj = maBmpList[ --i ];
> > So - maBmpList.erase( maBmpList.begin() + i ); is in fact pop_back and
> > it's effective but personally I'd use the latter to avoid mistakes.
>
> It's a loop and it's not just removing the last entry. It's only removing the entry that matches 
> the one passed. (I don't know why we're starting at the end since the same pointer shouldn't be 
> in the list twice; however, if the same pointer gets in the list twice, then we'll always remove 
> the last one instead of the first one and I didn't wont to change this behavior).
Ah, right, my fault. But now it's better to use list if you do not
need random access to elements. I mean maBmpList[ i ];

> Can you try the new version of the patch?
One more error:
maBmpList[ i ]->ImplRemovedFromCache(); should be
maBmpList[ i ]->mpBmp->ImplRemovedFromCache();
Now, just warning:
unx/generic/gdi/salbmp.cxx:1212: warning: ‘pObj’ may be used
uninitialized in this function
but that's not really true

I wonder if that was possible to compile for you but I have no
experience with other systems, so there may be big differences I'm not
used to.

Anyway, good job, I like removing old things or replacing them with new.

Matus