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

2011-07-20 Thread Matúš Kukan 
On 20 July 2011 04:51, Joseph Powers jpower...@cox.net wrote:

 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.

I'm sorry again. Did not realize that. (Or forgot).

 Hopefully this is the last time...
 Joe P.

Great.
So I'm not here to approve patches but originally you were asking for Unix build
and if you change #include List to #include list it will build
without any errors or warnings for me.
So probably you can push it.

Thanks,
Matus
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

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

2011-07-19 Thread Matúš Kukan 
Hi Joe,

On 19 July 2011 06:40, Joseph Powers jpower...@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:

 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.

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’
vcl/unx/generic/gdi/salbmp.cxx:1226: error: invalid use of incomplete
type ‘struct ImplSalBitmapCache::ImplBmpObj’
vcl/inc/unx/salbmp.h:253: error: forward declaration of ‘struct
ImplSalBitmapCache::ImplBmpObj’
vcl/unx/generic/gdi/salbmp.cxx:1227: error: invalid use of incomplete
type ‘struct ImplSalBitmapCache::ImplBmpObj’
vcl/inc/unx/salbmp.h:253: error: forward declaration of ‘struct
ImplSalBitmapCache::ImplBmpObj’
vcl/unx/generic/gdi/salbmp.cxx:1227: error: invalid use of incomplete
type ‘struct ImplSalBitmapCache::ImplBmpObj’
vcl/inc/unx/salbmp.h:253: error: forward declaration of ‘struct
ImplSalBitmapCache::ImplBmpObj’
vcl/unx/generic/gdi/salbmp.cxx:1230: error: invalid use of incomplete
type ‘struct ImplSalBitmapCache::ImplBmpObj’
vcl/inc/unx/salbmp.h:253: error: forward declaration of ‘struct
ImplSalBitmapCache::ImplBmpObj’
vcl/unx/generic/gdi/salbmp.cxx: In member function ‘void
ImplSalBitmapCache::ImplRemove(X11SalBitmap*)’:
vcl/unx/generic/gdi/salbmp.cxx:1240: error: invalid use of incomplete
type ‘struct ImplSalBitmapCache::ImplBmpObj’
vcl/inc/unx/salbmp.h:253: error: forward declaration of ‘struct
ImplSalBitmapCache::ImplBmpObj’
vcl/unx/generic/gdi/salbmp.cxx:1243: error: invalid use of incomplete
type ‘struct ImplSalBitmapCache::ImplBmpObj’
vcl/inc/unx/salbmp.h:253: error: forward declaration of ‘struct
ImplSalBitmapCache::ImplBmpObj’
vcl/unx/generic/gdi/salbmp.cxx:1244: error: invalid use of incomplete
type ‘struct ImplSalBitmapCache::ImplBmpObj’
vcl/inc/unx/salbmp.h:253: error: forward declaration of ‘struct
ImplSalBitmapCache::ImplBmpObj’
vcl/unx/generic/gdi/salbmp.cxx:1245: warning: possible problem
detected in invocation of delete operator:
vcl/unx/generic/gdi/salbmp.cxx:1239: warning: ‘pObj’ has incomplete type
vcl/inc/unx/salbmp.h:253: warning: forward declaration of ‘struct
ImplSalBitmapCache::ImplBmpObj’
vcl/unx/generic/gdi/salbmp.cxx:1245: note: neither the destructor nor
the class-specific operator delete will be called, even if they are
declared when the class is defined.
vcl/unx/generic/gdi/salbmp.cxx: In member function ‘void
ImplSalBitmapCache::ImplClear()’:
vcl/unx/generic/gdi/salbmp.cxx:1257: error: invalid use of incomplete
type ‘struct ImplSalBitmapCache::ImplBmpObj’
vcl/inc/unx/salbmp.h:253: error: forward declaration of ‘struct
ImplSalBitmapCache::ImplBmpObj’
vcl/unx/generic/gdi/salbmp.cxx:1258: warning: possible problem
detected in invocation of delete operator:
vcl/unx/generic/gdi/salbmp.cxx:1258: warning: invalid use of
incomplete type ‘struct ImplSalBitmapCache::ImplBmpObj’
vcl/inc/unx/salbmp.h:253: warning: forward declaration of ‘struct
ImplSalBitmapCache::ImplBmpObj’
vcl/unx/generic/gdi/salbmp.cxx:1258: note: neither the destructor nor
the class-specific operator delete will be called, even if they are
declared when the class is defined.

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

All the best,
Matus
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

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

2011-07-19 Thread Joseph Powers 

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

 Hi Joe,
 
 On 19 July 2011 06:40, Joseph Powers jpower...@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;
 
-ListmaBmpList;
+BmpList_implmaBmpList;
 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.


0001-Replace-List-with-std-vector-ImplBmpObj.patch
Description: Binary data
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

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

2011-07-19 Thread Matúš Kukan 
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 ];

 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
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

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

2011-07-19 Thread Joseph Powers 

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