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

[Joseph Powers]

On Jul 19, 2011, at 12:51 AM, Matúš Kukan wrote:

> Hi Joe,
> 
> On 19 July 2011 06:40, Joseph Powers <JPOWERS27 at cox.net> wrote:
>> I'd like someone doing a Unix build to review this for me. I compile Mac and this is Unix only code so I don't just want to push and hope...
>> 
> First I thought it would compile and want just to write something but
> then I tried and it doesn't.
> But my question is:
> Would not it be better to replace List with std::list ? Or if vector I
> don't like erase because it's not effective.
> In this case I'd use maBmpList.pop_back(). On the first sight I
> thought you have mistake in:

A List<> would  be better; however, it's a list of pointers so the size isn't that big.

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

> Now here is what I got on 32bit Ubuntu:
> 
> vcl/unx/generic/gdi/salbmp.cxx: In member function ‘void
> ImplSalBitmapCache::ImplAdd(X11SalBitmap*, sal_uLong, sal_uLong)’:
> vcl/unx/generic/gdi/salbmp.cxx:1218: error: invalid use of incomplete
> type ‘struct ImplSalBitmapCache::ImplBmpObj’
> vcl/inc/unx/salbmp.h:253: error: forward declaration of ‘struct
> ImplSalBitmapCache::ImplBmpObj’

Ok, just guessing but:

+struct ImplBmpObj;
+
 class ImplSalBitmapCache
 {
 private:
+    typedef ::std::vector< ImplBmpObj* > BmpList_impl;
 
-    List            maBmpList;
+    BmpList_impl    maBmpList;
     sal_uIntPtr     mnTotalSize;

Would most likely work better. I was defining "struct ImplBmpObj" inside the ImplSalBitmapCache class and in the .cxx the actual struct was defined outside the class; thus, we never defined the expected structure.

> I was not investigating where the problem is, I think you can handle it.

Can you try the new version of the patch?

> All the best,
> Matus

Thanks for helping,
Joe P.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Replace-List-with-std-vector-ImplBmpObj.patch
Type: application/octet-stream
Size: 4824 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20110719/09e87ce5/attachment-0001.obj>