Re: [PATCH] 2/2 List cleanup in SD
On Sunday 15 of April 2012, Rafael Dominguez wrote: Here are the patching fixing the issues you pointed me out. Yes, good, pushed, thanks again. One thing I didn't mention explicitly when talking about the named return value optimization was that it is done only if the named return variable is actually needed. If all return statements are expressions invoking constructors, then the named variable is not needed and the (non-named) return value optimization takes care of avoiding copies as well. So SdInsertPagesObjsDlg::GetList() can simply do if( ... ) return std::vectorrtl::OUString(); return aLbTree.GetSelectEntryList( nType ); which will avoid the copy when calling the other function as well, and the function is not called at all when not needed. I tweaked that before committing, now it's perfect :). -- Lubos Lunak l.lu...@suse.cz ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH] 2/2 List cleanup in SD
Here are the patching fixing the issues you pointed me out. 0001-Make-an-out-argument-a-pointer-instead-of-a-referenc.patch Description: Binary data 0002-Remove-deprecated-function.patch Description: Binary data 0003-Return-by-value-instead-of-passing-by-reference-to-g.patch Description: Binary data ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH] 2/2 List cleanup in SD
On Thursday 12 of April 2012, Rafael Dominguez wrote:
More cleanup of related functions.
I changed IterateBookmarkPages method to be a local function since its used
only in drawdoc3.cxx
0010-Remove-unused-InsertBookmarkAsPage_AddBookmarkedPage.patch
0011-Refactor-SdDrawDocument-IterateBookmarkPages.patch
[ build CXX ] sd/source/core/drawdoc3.cxx
/home/llunak/build/src/l2/sd/source/core/drawdoc3.cxx:80:7:
warning: 'InsertBookmarkAsPage_FindDuplicateLayouts' has virtual functions
but non-virtual destructor [-Wnon-virtual-dtor]
class InsertBookmarkAsPage_FindDuplicateLayouts
^
/home/llunak/build/src/l2/sd/source/core/drawdoc3.cxx:84:11: error:
initializer 'mpLayoutsToTransfer' does not name a non-static data member or
base class; did you mean the member 'mrLayoutsToTransfer'?
: mpLayoutsToTransfer(rLayoutsToTransfer) {}
^~~
mrLayoutsToTransfer
/home/llunak/build/src/l2/sd/source/core/drawdoc3.cxx:87:33:
note: 'mrLayoutsToTransfer' declared here
std::vectorrtl::OUString mrLayoutsToTransfer;
^
1 warning and 1 error generated.
I've removed the unnecessary virtual on the operator() and fixed the error
before pushing.
--
Lubos Lunak
l.lu...@suse.cz
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH] 2/2 List cleanup in SD
On Thursday 12 of April 2012, Rafael Dominguez wrote: This are the hardest ones to port, since the rExchangeList parameter uses the last position from InsertBookmarkAsPage in InsertBookmarkAsObject, so the best idea i came up with to avoid doing nasty stuffs was erase procesed objects in InsertBookmarkAsPage and add an extra parameter to InsertBookmarkAsObject that recalculates object count when rExchangeList has items but its manipulated by InsertBookmarkAsPage first like it occurs in function SdDrawDocument::InsertBookmark. Seems reasonable, to keep the behaviour of the old version. But the comment you added to the code next to the erasing probably doesn't explain much other than that the code is there for some reason, this comment above explains it better, so I've added it to the commit log message. Otherwise the patches looked good. -- Lubos Lunak l.lu...@suse.cz ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH] 2/2 List cleanup in SD
On Thursday 12 of April 2012, Rafael Dominguez wrote: Last patches, to test the Bookmark related functions, you need to insert another presentation or parts of a presentation by going into Insert-File. Thanks for checking all the patches!!! I have pushed all the remaining patches in this thread, I didn't see any problems in any of them. Thank you for all this work. It would be still nice to have the list returning cleaned up to return it normally instead of using an out argument, as already said before. Another somewhat stylistic nitpick is this: +std::vectorrtl::OUString aBookmarkList, aExchangeList; const SolarMutexGuard aGuard; bMergeMasterPages = (pDataDoc != rModel.GetDocument()); nInsertPageCount = pDataDoc-GetSdPageCount( PK_STANDARD ); rModel.GetDocument()-InsertBookmarkAsPage( -NULL, -NULL, +aBookmarkList, +aExchangeList, There's no need to create a named temporary variable for this, std::vectorrtl::OUString() in place of aBookmarkList would do and would make it more obvious that it is really just passing in an empty list that will not be used for anything. That would not work for the second argument, since the reference there is non-const, but if the argument was a pointer instead of a reference, that would both make it more visible that it is an optional argument (and NULL could be used there) and more visible that it is an out argument (aExchangeList makes it obvious that the variable will be changed by the function). Could you still please provide patches for these? Thanks. -- Lubos Lunak l.lu...@suse.cz ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH] 2/2 List cleanup in SD
On Wednesday 11 of April 2012, Rafael Dominguez wrote:
On Wed, Apr 11, 2012 at 2:53 PM, Caolán McNamara caol...@redhat.com wrote:
On Tue, 2012-04-10 at 12:52 +0200, Lubos Lunak wrote:
List* GetSelectEntryList( sal_uInt16 nDepth );
+voidGetSelectEntryList (sal_uInt16 nDepth,
std::vectorrtl::OUString rEntries) const;
Why is that? Changing the return value to a reference argument makes
the API worse and it seems like an unnecessary change to me.
...
Well the first reason is that you cant overload a function with a return
value,
Ok, this one didn't occur to me.
second reason and the main one, is that copying a vector is costly
so to prevent that i passes it by reference avoids that, but i can change
it later if needed.
Both gcc and msvc can do named return value optimization ([1][2]), so the
return object does not actually need to be copied. In a nutshell, if a
function as the first thing creates a local variable of the same type as the
return type and all return statements return this variable, then the compiler
will optimize by placing the variable directly in the place of the return
value, thus avoiding the copy. C++11's move semantics (will) make this moot
completely.
So, in code, a function like this does not create a copy of std::vector:
std::vector A foo( bool b )
{
std::vector A ret;
if( !b )
return ret; // return std::vectorA() would prevent the optimization
... // do things with ret;
return ret;
}
I see no good reason to delay your patches just because of this, but it would
be nice if you could do a followup patch to change such functions to return
values normally instead of the unneeded manual optimization (presumably with
a short suffix on one of the variants as long as both are needed).
[1] http://en.wikipedia.org/wiki/Return_value_optimization
[2] http://msdn.microsoft.com/en-us/library/ms364057(v=vs.80).aspx
--
Lubos Lunak
l.lu...@suse.cz
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PUSHED][PATCH] 2/2 List cleanup in SD
On Sun, 2012-04-01 at 09:19 -0430, Rafael Dominguez wrote: More cleanup of deprecated class List, need to be pushed after part1 and in order. Thanks! Please read carefully Seems sane, pushed now. Thanks for these. Sorry for the delay in getting around to them C. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH] 2/2 List cleanup in SD
On Tue, 2012-04-10 at 12:52 +0200, Lubos Lunak wrote: List* GetSelectEntryList( sal_uInt16 nDepth ); +voidGetSelectEntryList (sal_uInt16 nDepth, std::vectorrtl::OUString rEntries) const; Why is that? Changing the return value to a reference argument makes the API worse and it seems like an unnecessary change to me. Sorry, missed your earlier review. Yeah, struck me as well, but a quick attempt to remove all uses of the older one shows that there is a good bit of work still to do before e.g. removing the older GetSelectEntryList and making the new one return a vector, so the two still need to coexist for the moment. Maybe renaming the new GetSelectEntryList to say, getSelectEntryList, and have it return a vector, or let Rafael iterate through the rest of the nasty cases and change over afterwards. C. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH] 2/2 List cleanup in SD
On Wed, Apr 11, 2012 at 2:53 PM, Caolán McNamara caol...@redhat.com wrote: On Tue, 2012-04-10 at 12:52 +0200, Lubos Lunak wrote: List* GetSelectEntryList( sal_uInt16 nDepth ); +voidGetSelectEntryList (sal_uInt16 nDepth, std::vectorrtl::OUString rEntries) const; Why is that? Changing the return value to a reference argument makes the API worse and it seems like an unnecessary change to me. Sorry, missed your earlier review. Yeah, struck me as well, but a quick attempt to remove all uses of the older one shows that there is a good bit of work still to do before e.g. removing the older GetSelectEntryList and making the new one return a vector, so the two still need to coexist for the moment. Maybe renaming the new GetSelectEntryList to say, getSelectEntryList, and have it return a vector, or let Rafael iterate through the rest of the nasty cases and change over afterwards. C. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice Well the first reason is that you cant overload a function with a return value, second reason and the main one, is that copying a vector is costly so to prevent that i passes it by reference avoids that, but i can change it later if needed. I already finished the porting all the needed code, but didnt want to send it all because its alot of patches. I wanted another people review to double check i didnt mess it up. Can i send the rest of the patches? or maybe send it by parts?? Thanks for reviewing ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH] 2/2 List cleanup in SD
More cleanup of related functions. I changed IterateBookmarkPages method to be a local function since its used only in drawdoc3.cxx 0010-Remove-unused-InsertBookmarkAsPage_AddBookmarkedPage.patch Description: Binary data 0011-Refactor-SdDrawDocument-IterateBookmarkPages.patch Description: Binary data 0012-Remove-unneeded-headers.patch Description: Binary data 0014-Overload-lcl_IterateBookmarkPages-to-use-vector-inst.patch Description: Binary data ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH] 2/2 List cleanup in SD
This are the hardest ones to port, since the rExchangeList parameter uses the last position from InsertBookmarkAsPage in InsertBookmarkAsObject, so the best idea i came up with to avoid doing nasty stuffs was erase procesed objects in InsertBookmarkAsPage and add an extra parameter to InsertBookmarkAsObject that recalculates object count when rExchangeList has items but its manipulated by InsertBookmarkAsPage first like it occurs in function SdDrawDocument::InsertBookmark. Any input is highly appreciated! Thanks 0015-Overload-InsertBookmarkAsPage-to-use-std-vector-inst.patch Description: Binary data 0016-Overload-InsertBookmarkAsObject-to-use-std-vector-in.patch Description: Binary data ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH] 2/2 List cleanup in SD
More cleanup of related functions, this ones are easier. 0017-Make-some-inmutable-parameters-constant.patch Description: Binary data 0018-Replace-deprecated-List-with-std-vector-rtl-OUString.patch Description: Binary data 0019-Replace-deprecated-List-class-for-std-vector-rtl-OUS.patch Description: Binary data 0020-Overload-SdDrawDocument-InsertBookmark-to-use-vector.patch Description: Binary data ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH] 2/2 List cleanup in SD
Next in the series, to be able to compile you need to apply the whole set. This function affects copy/pasting slides. I tested it alot and also pasting from other presentations. 0021-Replace-deprecated-List-for-vector-in-SdTransferable.patch Description: Binary data 0022-Remove-deprecated-List-usage-in-Clipboard-PasteTrans.patch Description: Binary data 0023-Remove-deprecated-List-usage-in-Clipboard-CreateSlid.patch Description: Binary data 0024-Call-size-instead-of-count.patch Description: Binary data 0025-Remove-deprecated-List-for-vector-in-ViewClipboard-G.patch Description: Binary data 0026-Remove-deprecated-List-in-ViewClipboard-InsertSlides.patch Description: Binary data ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH] 2/2 List cleanup in SD
On Sunday 01 of April 2012, Rafael Dominguez wrote: More cleanup of deprecated class List, need to be pushed after part1 and in order. Thanks! Please read carefully It seems nobody has handled this yet. I have checked the patches and they look good to me, except for one thing that I'd like to clear up before committing. The patches do changes like this: List* GetSelectEntryList( sal_uInt16 nDepth ); +voidGetSelectEntryList (sal_uInt16 nDepth, std::vectorrtl::OUString rEntries) const; Why is that? Changing the return value to a reference argument makes the API worse and it seems like an unnecessary change to me. -- Lubos Lunak l.lu...@suse.cz ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
[PATCH] 2/2 List cleanup in SD
More cleanup of deprecated class List, need to be pushed after part1 and in order. Thanks! Please read carefully 0006-Remove-usage-of-List-in-InsertBookmarkAsPage_FindDup.patch Description: Binary data 0007-Replace-deprecated-List-class-for-std-map-sal_uInt16.patch Description: Binary data 0008-Replace-deprecated-List-for-std-vector-SdPage.patch Description: Binary data 0009-Replace-deprecated-List-with-std-vector-StyleReplace.patch Description: Binary data ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
