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

[Joseph Powers]

On Jul 19, 2011, at 7:34 AM, Matúš Kukan wrote:

> On 19 July 2011 14:22, Joseph Powers <JPOWERS27 at 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 ];

Ok, I changed from a stl::vector<> to a stl::list<>; I also rewrote the loops to use an iterator instead of [] addressing since [] addressing can be expensive with lists. I also rewrote the loop in question to find the 1st match (there should only be one match) because it makes the logic cleaner and easier to read.

>> 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

Ok, I missed that one.  I also added code to initialize pObj to NULL; some of the developers like to compile with "error on warnings" and they'ed get really mad if we left a warning.

> 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.

I only compile for Mac OS; thus, I only compile /vcl/aqua & /vcl/source. If you watch your compile, you should only compile /vcl/unx & /vcl/source. We also have /vcl/win for those brave people who compile on Windows.

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

Hopefully this is the last time...
Joe P.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Replace-List-with-std-list-ImplBmpObj.patch
Type: application/octet-stream
Size: 5159 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20110719/f8d46c06/attachment.obj>
-------------- next part --------------