On Jul 19, 2011, at 12:51 AM, Matúš Kukan wrote:
Hi Joe, On 19 July 2011 06:40, Joseph Powers <JPOWERS27@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.
Attachment:
0001-Replace-List-with-std-vector-ImplBmpObj.patch
Description: Binary data