On Jul 19, 2011, at 7:34 AM, Matúš Kukan wrote:
> On 19 July 2011 14:22, Joseph Powers <jpower...@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.
0001-Replace-List-with-std-list-ImplBmpObj.patch
Description: Binary data
_______________________________________________ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
